Bug 28410

Summary: [Omnibus] Reduce memory footprint
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Bugs List <koha-bugs>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, julian.maurice, martin.renvoize, nick, philippe.blouin, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35250
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 27267, 28411, 28415, 34477, 28306, 28413, 28416, 28417    
Bug Blocks:    
Attachments: Bug 28410: Use lazy loading to reduce memory footprint of C4/Context.pm
Bug 28410: Use lazy loading to reduce memory footprint of C4/Context.pm
Bug 28410: remove Koha::Config::Syspref(s) from C4::Context
Bug 28410: remove Koha::Config::Syspref(s) from C4::Context
Bug 28410: remove Koha::Config::Syspref(s) from C4::Context

Description Jonathan Druart 2021-05-21 11:47:28 UTC
Mostly because of our circular dependencies we are facing memory issue. Like any simple scripts will have to load all the modules.

We should focus on reducing the memory footprint.
Comment 1 David Cook 2021-05-24 00:24:02 UTC
(In reply to Jonathan Druart from comment #0)
> We should focus on reducing the memory footprint.

+1000
Comment 2 Blou 2023-10-24 17:17:35 UTC
Do you mind explaining the "circular dependencies" and how it impacts the footprint?  I'd be happy to look at that.

Any simple script (cron jobs) is having an impact on CPU when launched, just because of its "size".
Comment 3 Blou 2023-10-24 18:43:49 UTC
Ok, I digged in v23

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



Since everything goes back to a Koha::Object or C4::Context, we always start with 30M in three external libraries, plus the 1-2M we get for every other module.


Please note that my simple script total 57M in v23.05.  Redoing it in v22 I get 150M, but I don't see the point right now.  And I get more in different environment, maybe because of my database content?
Comment 4 Blou 2023-10-24 19:48:28 UTC
I know I might sound blasphemous, but to me C4::Context should not carry this bagage, even at the cost of the nice OO design.

I want the context.  I think this includes the config file and the sysprefs, but this should not automatically get the whole schema.  And the only way to do that would be to access the sysprefs without using Koha::Config::Syspref.  Thus no Action::Log, no Koha::Object.


I think the structure makes sense OO-wise, but as the great Scott Meyers (shit, I'm old) said : "Non-member functions improve encapsulation".  In this case, it's more about pragmatism.  A few old-school SELECT in C4::Context, and you reduce all your scripts memory footprints by a lot.  

Same with ZOOM?  Does it make sense in C4::Context, in 2023 ?

And Datetime::TimeZone... just to call C4::Context->tz in a few context.  This could be in DateUtils, asking the context from C4::Context, instead of C4::Context carrying all that for a few calls.


Maybe I'm naive, but it seems rather simple to reduce 40M of RES out of most Koha scripts.
Comment 5 David Cook 2023-10-24 23:03:21 UTC
(In reply to Blou from comment #4)
> Maybe I'm naive, but it seems rather simple to reduce 40M of RES out of most
> Koha scripts.

I think it's easier said than done. Sounds like a fair number of changes, and each one needs testing, QAing, and pushing, which can take a lot of effort.

That said, it's worth doing! And it sounds like there are at least 3 of us interested in this, so I think we could get some changes done.

--

The hard part about these changes is that they affect all of Koha, which makes them big impact.
Comment 6 David Cook 2023-10-24 23:08:51 UTC
(In reply to Blou from comment #4)
>  A few old-school SELECT in
> C4::Context, and you reduce all your scripts memory footprints by a lot.  

That's the strategy I use in bug 27267 and it is highly effective. 

I need to rewrite the patch on that bug (if only I had infinite time).

I think the background jobs worker would benefit from the same approach. In the RabbitMQ codeflow, the database lookup is very simple. It could easily be done without an ORM.

The DB codeflow is a bit messier since it takes an arrayref for "queue". It can still be done in plain SQL but is harder to do.
Comment 7 David Cook 2023-10-24 23:10:19 UTC
(In reply to Blou from comment #4)
> I know I might sound blasphemous, but to me C4::Context should not carry
> this bagage, even at the cost of the nice OO design.

We could also use lazy loading in C4::Context to reduce the memory impact of just loading C4::Context.
Comment 8 Blou 2023-10-25 21:03:29 UTC
Created attachment 157875 [details] [review]
Bug 28410: 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 9 Blou 2023-10-25 21:14:42 UTC
I followed on David's comment and tried a few lazy loading.  Some have a good impact (like ZOOM), but others like the TimeZone have no impact because it's included in some other module in the Koha::Caches or Koha::Config hierarchy. 

Lazy loading Koha::Config makes no sense, and Cache is cycling back to Context, funnily.

I can try some ugly code for the preferences, as a POC.  I hope to have time this week, if no one else give it a go.
Comment 10 David Cook 2023-10-25 22:14:56 UTC
(In reply to Blou from comment #9)
> I followed on David's comment and tried a few lazy loading.  Some have a
> good impact (like ZOOM), but others like the TimeZone have no impact because
> it's included in some other module in the Koha::Caches or Koha::Config
> hierarchy. 

Have you run the test suite? We need to make sure that no other code was relying on C4::Context loading those modules when it's loaded.

> Lazy loading Koha::Config makes no sense, and Cache is cycling back to
> Context, funnily.

That's one of those circular dependencies. 

> I can try some ugly code for the preferences, as a POC.  I hope to have time
> this week, if no one else give it a go.

So bug 28410 isn't the right place to put patches. You'll want to raise separate tickets and then link them using "Depends on".
Comment 11 Blou 2023-10-26 12:50:58 UTC
> So bug 28410 isn't the right place to put patches. You'll want to raise
> separate tickets and then link them using "Depends on".

Yeah, I figured that, but I was really putting them here for your eyes mostly.  More as a sample into a journey of discovery than as a real patch.  It's not obvious if it's the way you really want to do it.

I'll run the test suite before and after the next one.
Comment 12 Blou 2023-10-26 18:10:21 UTC
Created attachment 157945 [details] [review]
Bug 28410: 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 13 Blou 2023-10-26 18:10:24 UTC
Created attachment 157946 [details] [review]
Bug 28410: remove 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
Comment 14 Jonathan Druart 2023-10-26 19:14:17 UTC
We are loosing the logaction calls.
Comment 15 Blou 2023-10-26 19:25:00 UTC
(In reply to Jonathan Druart from comment #14)
> We are loosing the logaction calls.

Very good point.  

This needs to be preserved, one way or another.  It's unfortunate the call to "preference" being to prevalent everywhere, it makes it hard to move all that elsewhere.  Koha::Config::Sysprefs could have been the entry point otherwise.
Comment 16 Blou 2023-10-26 20:07:30 UTC
Created attachment 157952 [details] [review]
Bug 28410: remove 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
Comment 17 Blou 2023-10-26 20:19:47 UTC
Created attachment 157953 [details] [review]
Bug 28410: remove 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
Comment 18 Blou 2023-10-26 20:22:21 UTC
prove t/db_dependent/sysprefs.t fails the cache test.
Comment 19 Blou 2023-10-26 20:28:08 UTC
moved my patches to Bug 28411, more appropriate as pointed out.
Comment 20 Jonathan Druart 2023-10-27 05:43:12 UTC
(In reply to Blou from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > We are loosing the logaction calls.
> 
> Very good point.  
> 
> This needs to be preserved, one way or another.  It's unfortunate the call
> to "preference" being to prevalent everywhere, it makes it hard to move all
> that elsewhere.  Koha::Config::Sysprefs could have been the entry point
> otherwise.

Yes, this is what need to be done here (remove C4::Context->preference). It could be scripted.
I was sure we had a bug for that but I didn't find it.
Comment 21 Blou 2023-10-27 14:19:57 UTC
Did a quick check.  There are .... a lot of them.

One question: installer/data/mysql/updatedatabase.pl calls C4::Context->preference, which is now an object call behind the scene.  I know updatedatabase is not the do-it-all anymore, but to ensure it still works for years to come shouldn't we replace those calls by a local, straight SQL call ?

A few hundred calls to preference, and one to set_preference.


THAT SAID, pragmatism tells me to look for another way: Modify preference() to use straight DB, and move all the calls to set_preference() and delete_preference() to Koha::Config::Sysprefs.  

I'll test that in Bug 28411.  Won't prevent major overhaul later, but seems like a safe approach.
Comment 22 Jonathan Druart 2023-10-27 14:59:50 UTC
(In reply to Blou from comment #21)
> Did a quick check.  There are .... a lot of them.
> 
> One question: installer/data/mysql/updatedatabase.pl calls
> C4::Context->preference, which is now an object call behind the scene.  I
> know updatedatabase is not the do-it-all anymore, but to ensure it still
> works for years to come shouldn't we replace those calls by a local,
> straight SQL call ?
> 
> A few hundred calls to preference, and one to set_preference.
> 
> 
> THAT SAID, pragmatism tells me to look for another way: Modify preference()
> to use straight DB, and move all the calls to set_preference() and
> delete_preference() to Koha::Config::Sysprefs.  
> 
> I'll test that in Bug 28411.  Won't prevent major overhaul later, but seems
> like a safe approach.

Why do you want to remove C4::Context from updatedatabase? What would be the gain?
Comment 23 Blou 2023-10-27 15:13:21 UTC
We were talking about moving all calls to preference().

updatedatabase.pl has had problems in the past when the code calls DB objects of the new version that do not correspond to the database that is currently being upgraded.

So if systempreference gets modified, along with Koha::Config::Syspref (?) in 26.05, calling C4::Context->(set_)preference during the ugprade process on a database that is at 21.05 at that point... will crash, when using the underlying object.

No?  Maybe systempreference doesn't work like that, or maybe you have a new way of doing thing.
Comment 24 David Cook 2023-10-29 22:30:08 UTC
(In reply to Jonathan Druart from comment #22)
> Why do you want to remove C4::Context from updatedatabase? What would be the
> gain?

While I don't directly accuse C4::Context in bug 34088, it is one reason for slowness when running updatedatabase.pl against a large number of instances. 

Philippe, you might find that bug interesting. That one isn't for the memory footprint per se but rather speed.
Comment 25 Jonathan Druart 2023-10-31 08:17:21 UTC
Philippe, please move your patch to its own bug, and link it to this one.

So that we can discuss on this approach specifically on this bug and it does not get lost here.

I would suggest to keep the *pref* subs in C4::Context, that will neither use the cache nor the Koha::Config::SysPref module (and so the dbic schema won't be loaded).

Should be marked *deprecated do not use*, but at least we won't break plugins, etc.
Comment 26 Blou 2023-10-31 12:12:49 UTC
I did that (comment 21).  Moved everything to Bug 28411, seemed more appropriate.  There are no patch left in this bug.

Good point about the plugins, didn't think of that.
Comment 27 Jonathan Druart 2023-10-31 13:20:18 UTC
(In reply to Blou from comment #26)
> I did that (comment 21).  Moved everything to Bug 28411, seemed more
> appropriate.  There are no patch left in this bug.
> 
> Good point about the plugins, didn't think of that.

Yes, the comment was for 28411. I thought I was commenting there, sorry.
Please move to its own bug (specific to C4::Context->preference, not circ deps).