Bug 35250 - Eliminate circular dependencies caused by C4::Context
Summary: Eliminate circular dependencies caused by C4::Context
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Blou
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 28411
  Show dependency treegraph
 
Reported: 2023-11-03 15:22 UTC by Blou
Modified: 2024-04-12 20:40 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 35250: Use lazy loading to reduce memory footprint of C4/Context.pm (2.35 KB, patch)
2023-11-03 19:04 UTC, Blou
Details | Diff | Splinter Review
Bug 35250: remove usages of Koha::Config::Syspref(s) from C4::Context (7.47 KB, patch)
2023-11-03 19:04 UTC, Blou
Details | Diff | Splinter Review
Bug 35250: modify scripts calling C4::Context::set_preference (10.41 KB, patch)
2023-11-03 19:04 UTC, Blou
Details | Diff | Splinter Review
Bug 35250: modify t/db_dependent tests to use Koha::Config::SysPrefs' set_preference instead of C4::Context (28.53 KB, patch)
2023-11-03 19:04 UTC, Blou
Details | Diff | Splinter Review
Bug 35250: Use lazy loading to reduce memory footprint of C4/Context.pm (2.35 KB, patch)
2023-12-12 13:18 UTC, Salah Ghedda
Details | Diff | Splinter Review
Bug 35250: remove usages of Koha::Config::Syspref(s) from C4::Context (7.47 KB, patch)
2023-12-12 13:18 UTC, Salah Ghedda
Details | Diff | Splinter Review
Bug 35250: modify scripts calling C4::Context::set_preference (10.41 KB, patch)
2023-12-12 13:18 UTC, Salah Ghedda
Details | Diff | Splinter Review
Bug 35250: modify t/db_dependent tests to use Koha::Config::SysPrefs' set_preference instead of C4::Context (28.65 KB, patch)
2023-12-12 13:18 UTC, Salah Ghedda
Details | Diff | Splinter Review
Bug 35250: Use lazy loading to reduce memory footprint of C4/Context.pm (2.36 KB, patch)
2024-03-22 18:24 UTC, Blou
Details | Diff | Splinter Review
Bug 35250: remove usages of Koha::Config::Syspref(s) from C4::Context (7.47 KB, patch)
2024-03-22 18:24 UTC, Blou
Details | Diff | Splinter Review
Bug 35250: modify scripts calling C4::Context::set_preference (10.46 KB, patch)
2024-03-22 18:24 UTC, Blou
Details | Diff | Splinter Review
Bug 35250: modify t/db_dependent tests to use Koha::Config::SysPrefs' set_preference instead of C4::Context (27.36 KB, patch)
2024-03-22 18:24 UTC, Blou
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2023-11-03 15:22:34 UTC
Any script using C4::Context is bound to have a huge memory footprint because of the dependencies.  And even script that would do without C4::Context (somehow) are bound for the same as soon as they include one Koha:: module, because those will use C4::Context.

A graph from v23.05 will summarize it better than any explanation:

BackgroundJobs
|
v
BackgroundJob  (Net::Stomp : 5M)
|   |
|   v
|   Koha::Object  (Mojo::JSON : 16M)
v
C4::Context   (DateTime::TimeZone : 7M)
|    |        (ZOOM : 8M)
v    |
Koha::Config::Syspref
|    |
v    |
C4::Log  (calling back on C4::Context)
|    |
v    |
Koha::ActionLogs
|    |
v    |
Koha::ActionLog (calling back on C4::Context)
|    |
v    |
Koha::Object
     |
     |
     v
     Koha::Caches (calling back on C4::Context)
Comment 1 Blou 2023-11-03 15:24:47 UTC
This is a branch from Bug 28410 and Bug 28411, for earlier exchanges.
Comment 2 Blou 2023-11-03 19:04:40 UTC
Created attachment 158361 [details] [review]
Bug 35250: Use lazy loading to reduce memory footprint of C4/Context.pm

This simply removes some 'use' declarations at the top of the file to
instead use 'require' later on.

The effect can be validated with a simple one-liner (provided KOHA_CONF
and PERL5LIB being set)

perl -e "use C4::Context; while(1){sleep(1);}"

And monitoring the memoring usage in tools like 'top'
Comment 3 Blou 2023-11-03 19:04:42 UTC
Created attachment 158362 [details] [review]
Bug 35250: remove usages of Koha::Config::Syspref(s) from C4::Context

Use straigth SQL queries to access systempreferences instead of relying
on the ORM, thus removing all direct and indirect dependencies to
Koha::Object

To remove all dependencies between C4::Context and the db object model,
we move the calls to set_preference and delete_prefence directly to the
class object Koha::Config::SysPrefs

C4::Context::preference remains but uses straight SQL
C4::Context::set_preference moves to Koha::Config::SysPrefs::set_preference
C4::Context::delete_preference moves to Koha::Config::SysPrefs::delete_preference
Comment 4 Blou 2023-11-03 19:04:44 UTC
Created attachment 158363 [details] [review]
Bug 35250: modify scripts calling C4::Context::set_preference

All the scripts using the call C4::Context->set_preference are edited to
now use Koha::Config::SysPrefs->set_preference instead of C4::Context->set_preference()

One call to delete_preference moved as well
Comment 5 Blou 2023-11-03 19:04:46 UTC
Created attachment 158364 [details] [review]
Bug 35250: modify t/db_dependent tests to use Koha::Config::SysPrefs' set_preference instead of C4::Context

Some work left to be done, including missing use at top of files
Comment 6 Salah Ghedda 2023-12-12 13:18:25 UTC
Created attachment 159735 [details] [review]
Bug 35250: Use lazy loading to reduce memory footprint of C4/Context.pm

This simply removes some 'use' declarations at the top of the file to
instead use 'require' later on.

The effect can be validated with a simple one-liner (provided KOHA_CONF
and PERL5LIB being set)

perl -e "use C4::Context; while(1){sleep(1);}"

And monitoring the memoring usage in tools like 'top'
Comment 7 Salah Ghedda 2023-12-12 13:18:27 UTC
Created attachment 159736 [details] [review]
Bug 35250: remove usages of Koha::Config::Syspref(s) from C4::Context

Use straigth SQL queries to access systempreferences instead of relying
on the ORM, thus removing all direct and indirect dependencies to
Koha::Object

To remove all dependencies between C4::Context and the db object model,
we move the calls to set_preference and delete_prefence directly to the
class object Koha::Config::SysPrefs

C4::Context::preference remains but uses straight SQL
C4::Context::set_preference moves to Koha::Config::SysPrefs::set_preference
C4::Context::delete_preference moves to Koha::Config::SysPrefs::delete_preference
Comment 8 Salah Ghedda 2023-12-12 13:18:29 UTC
Created attachment 159737 [details] [review]
Bug 35250: modify scripts calling C4::Context::set_preference

All the scripts using the call C4::Context->set_preference are edited to
now use Koha::Config::SysPrefs->set_preference instead of C4::Context->set_preference()

One call to delete_preference moved as well
Comment 9 Salah Ghedda 2023-12-12 13:18:31 UTC
Created attachment 159738 [details] [review]
Bug 35250: modify t/db_dependent tests to use Koha::Config::SysPrefs' set_preference instead of C4::Context

Some work left to be done, including missing use at top of files
Comment 10 Salah Ghedda 2023-12-12 13:18:55 UTC
Rebased
Comment 11 Blou 2024-03-22 18:24:01 UTC
Created attachment 163733 [details] [review]
Bug 35250: Use lazy loading to reduce memory footprint of C4/Context.pm

This simply removes some 'use' declarations at the top of the file to
instead use 'require' later on.

The effect can be validated with a simple one-liner (provided KOHA_CONF
and PERL5LIB being set)

perl -e "use C4::Context; while(1){sleep(1);}"

And monitoring the memoring usage in tools like 'top'
Comment 12 Blou 2024-03-22 18:24:03 UTC
Created attachment 163734 [details] [review]
Bug 35250: remove usages of Koha::Config::Syspref(s) from C4::Context

Use straigth SQL queries to access systempreferences instead of relying
on the ORM, thus removing all direct and indirect dependencies to
Koha::Object

To remove all dependencies between C4::Context and the db object model,
we move the calls to set_preference and delete_prefence directly to the
class object Koha::Config::SysPrefs

C4::Context::preference remains but uses straight SQL
C4::Context::set_preference moves to Koha::Config::SysPrefs::set_preference
C4::Context::delete_preference moves to Koha::Config::SysPrefs::delete_preference
Comment 13 Blou 2024-03-22 18:24:05 UTC
Created attachment 163735 [details] [review]
Bug 35250: modify scripts calling C4::Context::set_preference

All the scripts using the call C4::Context->set_preference are edited to
now use Koha::Config::SysPrefs->set_preference instead of C4::Context->set_preference()

One call to delete_preference moved as well
Comment 14 Blou 2024-03-22 18:24:07 UTC
Created attachment 163736 [details] [review]
Bug 35250: modify t/db_dependent tests to use Koha::Config::SysPrefs' set_preference instead of C4::Context

Some work left to be done, including missing use at top of files
Comment 15 Jonathan Druart 2024-03-25 12:26:33 UTC
Replacing use with require is not recommended. IMO the correct fix here would be to move sysprefs code outside of C4::Context.
Comment 16 Jonathan Druart 2024-03-25 12:29:08 UTC
Sorry, it's what you are actually doing in the other patches.

I think that Koha::Config::Syspref should not use C4::Context, so that you could fetch sysprefs without the whole memory stack.
Comment 17 Blou 2024-03-25 13:43:32 UTC
If that's allowed, I'll explore that.

I just got back to it (thus the recent rebase) and applied them on our 23.05s to validate nothing unexpected happens (and I need some RES memory back on our servers :-) )
Comment 18 Blou 2024-04-12 20:40:34 UTC
It is very hard to decouple those fully.  Config::SysPrefs uses $C4::Context::use_syspref_cache, which is in C4::Context because the sub preference() is still there and might use the cache.

Of course, all this could/should be moved to Koha/Config/SysPrefs, but we're talking hundred of calls to change from C4::Context->preference to Koha::Config::SysPrefs->preference(), and that would be hell to rebase repetitively.


Right now (locally), I get a 50M footprint to simple use of Koha::Config::SysPrefs, and 21M for C4::Context.  I don't think SysPrefs can live without C4/Context because how would it find its DB connection?  Somewhere, SysPrefs needs Context.  But at least, Context doesn't need SysPrefs and that would make a big difference to a lot of scripts if we could decouple Context completely from the db objects.  If I understand right.


In short, the only thing I see I could do "more" is moving the preference subs to SysPrefs, but that seems like a task to automate just before a Pushed to Master, not something that can be rebased for months.