Tryton - Issues

 

Issue8749

Title Fetching config values can be a performance bottleneck
Priority feature Status resolved
Superseder Nosy List ced, jcavallo, nicoe, pokoli, reviewbot, roundup-bot
Type performance Components trytond
Assigned To ced Keywords review
Reviews 284271002, 250131002,276511002
View: 284271002, 250131002, 276511002

Created on 2019-10-22.10:31:45 by nicoe, last changed by roundup-bot.

Messages
New changeset ea5722c2d3c1 by Cédric Krier in branch 'default':
Avoid to import backend before loading configuration
https://hg.tryton.org/tryton-env/rev/ea5722c2d3c1
New changeset 173093bcb7b2 by Cédric Krier in branch 'default':
Avoid to import backend before loading configuration
https://hg.tryton.org/trytond/rev/173093bcb7b2
review276511002 updated at https://codereview.tryton.org/276511002/#ps264841003
review276511002 updated at https://codereview.tryton.org/276511002/#ps258481002
msg53730 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2019-11-29.10:30:29
I re-open because run-tests does not honor any more the configuration of the backend. Here is review276511002 which fix that.
New changeset 2f98b12f48ff by Cédric Krier in branch 'default':
Import admin and console after application is configured
https://hg.tryton.org/tryton-env/rev/2f98b12f48ff
New changeset 814183c07ae1 by Cédric Krier in branch 'default':
Import admin and console after application is configured
https://hg.tryton.org/trytond/rev/814183c07ae1
New changeset 07279da075a9 by Nicolas ?vrard in branch 'default':
Cache some performance sensible config values at the module level
https://hg.tryton.org/tryton-env/rev/07279da075a9
New changeset 953fa4029dc3 by Nicolas ?vrard in branch 'default':
Cache some performance sensible config values at the module level
https://hg.tryton.org/trytond/rev/953fa4029dc3
review284271002 updated at https://codereview.tryton.org/284271002/#ps282421004
msg53325 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2019-11-18.17:45:22
* Cédric Krier [2019-10-23 15:32:18]:
> 
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
> 
> I looked at the ConfigParser implementation and I do not see a lot
> of work done there except by the BasicInterpolation.
> So I think it is good to convert into const the three configuration
> value of the ORM: trytond/transaction.py,
> trytond/backend/__init__.py and trytond/model/modestorage.py
> And to use the Dummy Interpolation instead of BasicInterpolation for
> the ConfigParser.

The last patch implements just that.

With one change: RawConfigParser uses the dummy Interpolation already
(as per
https://docs.python.org/3/library/configparser.html#configparser.RawConfigParser
and
https://github.com/python/cpython/blob/f49f6baa6bf7916ac039194c24b59d2eff5b180a/Lib/configparser.py#L587)
msg52803 (view) Author: [hidden] (jcavallo) Date: 2019-10-30.14:26:32
I did some more tests by manually patching the server in a dev environment, and I
think that the best compromise would indeed be to use consts loaded from the
configuration file for the ORM-related informations.

This is a rather small change, and it solves the slowdown I encountered when
manipulating a large number of records.
msg52650 (view) Author: [hidden] (jcavallo) Date: 2019-10-23.15:47:15
> So I think it is good to convert into const the three configuration value of
> the ORM: trytond/transaction.py, trytond/backend/__init__.py and trytond/model/modestorage.py
> The other value can stay as they are for now except if we can spot that it is
> a very frequently used code.

I think those are indeed the values that will benefit the most from a caching
mechanism, whatever the solution ends up to be.
msg52649 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2019-10-23.15:32:18
I looked at the ConfigParser implementation and I do not see a lot of work done there except by the BasicInterpolation.
So I think it is good to convert into const the three configuration value of the ORM: trytond/transaction.py, trytond/backend/__init__.py and trytond/model/modestorage.py
And to use the Dummy Interpolation instead of BasicInterpolation for the ConfigParser.
The other value can stay as they are for now except if we can spot that it is a very frequently used code.
msg52647 (view) Author: [hidden] (jcavallo) Date: 2019-10-23.13:58:05
I tried to test your second review, but it does not work properly,
because when updating the configuration through the trytond.conf
file, ConfigParser does not call set but rather directly updates
the internal variables. So the only values I get when starting the
server are the default values.
New review250131002 at https://codereview.tryton.org/250131002/#ps270371002
msg52645 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2019-10-23.13:22:29
* Jean CAVALLO [2019-10-22 15:18:26]:
> 
> Jean CAVALLO <jean.cavallo@coopengo.com> added the comment:
> 
> >> I'd probably be more in favor of integrating a cache directly in
> >> TrytonConfigParser,
> 
> > Yes me too but the fear is that there would be no gain by using a
> > cache on this level. Would you be ready to test both implementation
> > and report back?
> 
> I would, my use case only requires the trytond patch, since it's mostly the
> cache / record-model-field caches that are taking some time.

I made a second patch with the cache on the config parser level (not
tested on the modules yet).

Here are some raw results on the trytond tests (few care has been
taken when compulsing those figures :P):

    - without any cache: 34.988s, 35.081s, 34.448s, 34.905s
    - cache on the module level: 33.895s, 33.040s, 33.107s, 34.028s
    - cache on the config level: 33.048s, 33.714s, 33.782s, 33.703s

So obviously the cache on the module level is a bit faster. But is the
gain worth the introduction of those global variables?
review284271002 updated at https://codereview.tryton.org/284271002/#ps286131002
msg52643 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2019-10-23.11:08:25
* Cédric Krier [2019-10-23 10:04:53]:
> 
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
> 
> > Some values are not cacheable.
> 
> Which one?

It's not the value per se that is not cacheable but rather the context
that make it not cacheable.

eg: Sometimes the key is computed or defined by a value in the database.

Like in stock_package_shipping_ups:

    https://hg.tryton.org/modules/stock_package_shipping_ups/file/cd2a587eecd8/stock.py#l142

Or in trytond/ir/routes.py (the implementation of get_config computes
the key).

> > I'm wondering if it should be usefull to add (or at least discuss)
> > on the python language.
> 
> It could but we will benefit only in 1.5 years (at best). And it
> does not make that using constant value in top would be faster
> (local lookup) than calling a method.

The delay is a very good reason. Also I think that if we have an
implementation to demonstrate with some gains it would give us a
leverage when submitting the patch upstream.
msg52640 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2019-10-23.10:04:52
> Some values are not cacheable.

Which one?

> I'm wondering if it should be usefull to add (or at least discuss) on the python language.

It could but we will benefit only in 1.5 years (at best). And it does not make that using constant value in top would be faster (local lookup) than calling a method.
msg52618 (view) Author: [hidden] (jcavallo) Date: 2019-10-22.16:06:27
For information, here are the relevant parts of the profiling 

INFO:trytond.autoprofile:*** PROFILER RESULTS ***
INFO:trytond.autoprofile:_compute_premiums_get_schedule
INFO:trytond.autoprofile:function called 1 times
INFO:trytond.autoprofile:
INFO:trytond.autoprofile:         5519903 function calls (4946427 primitive calls) in 4.985 seconds
INFO:trytond.autoprofile:
INFO:trytond.autoprofile:   Ordered by: cumulative time
INFO:trytond.autoprofile:   List reduced from 860 to 80 due to restriction <80>
INFO:trytond.autoprofile:
INFO:trytond.autoprofile:   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
INFO:trytond.autoprofile:        1    0.000    0.000    4.986    4.986 api.py:884(_compute_premiums_get_schedule)
INFO:trytond.autoprofile:    21650    0.171    0.000    1.465    0.000 modelstorage.py:1494(instantiate)
INFO:trytond.autoprofile:    44520    0.060    0.000    0.521    0.000 config.py:144(getint)
INFO:trytond.autoprofile:    44520    0.053    0.000    0.453    0.000 configparser.py:815(getint)
INFO:trytond.autoprofile:    44520    0.050    0.000    0.400    0.000 configparser.py:804(_get_conv)
INFO:trytond.autoprofile:    44520    0.055    0.000    0.350    0.000 configparser.py:801(_get)
INFO:trytond.autoprofile:    44679    0.059    0.000    0.296    0.000 config.py:136(get)
INFO:trytond.autoprofile:    44679    0.073    0.000    0.230    0.000 configparser.py:764(get)


When hardcoding the cache related values in getint, the only line related to the
config that remains is:

INFO:trytond.autoprofile:     2919    0.005    0.000    0.126    0.000 cache.py:323(get)

With the current patch (manually applied on our 5.2 fork, so there may have been
some errors :) ):

INFO:trytond.autoprofile:     2919    0.005    0.000    0.119    0.000 cache.py:324(get)
msg52614 (view) Author: [hidden] (jcavallo) Date: 2019-10-22.15:18:26
>> I'd probably be more in favor of integrating a cache directly in
>> TrytonConfigParser,

> Yes me too but the fear is that there would be no gain by using a
> cache on this level. Would you be ready to test both implementation
> and report back?

I would, my use case only requires the trytond patch, since it's mostly the
cache / record-model-field caches that are taking some time.

> Did we have some script to test: the current performance and the performance with both patches? 

I noticed the problem when testing a very demanding API in our custom application,
and got to the point where, out of a 5 second call, about 0.5 second (10%) was
spent in config.getint

AFAIU, this was mainly caused by trytond loading the cache size informations
when creating the local cache when instantiating new records (I have about 50k
instantiations in this call).

Basically, manually patching getint to hardcode with a simple if / elif the
cache related values gave those 0.5 seconds back.
msg52612 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2019-10-22.15:09:16
I agree with msg52605 but I'm wondering if it should be usefull to add (or at least discuss) on the python language. IIRC the TrytonConfigParser is a default configParser with only a sensible defaults. So any performance improvement on python will benefit tryton also. 

Did we have some script to test: the current performance and the performance with both patches? 

I think this information will be very usefull to decide which is the best solution.
msg52610 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2019-10-22.14:23:06
* Jean CAVALLO [2019-10-22 10:57:53]:
> 
> Jean CAVALLO <jean.cavallo@coopengo.com> added the comment:
> 
> I'd probably be more in favor of integrating a cache directly in
> TrytonConfigParser,

Yes me too but the fear is that there would be no gain by using a
cache on this level. Would you be ready to test both implementation
and report back?
msg52605 (view) Author: [hidden] (jcavallo) Date: 2019-10-22.10:57:53
I'd probably be more in favor of integrating a cache directly in
TrytonConfigParser, but the current approach allows to decided
value per value if we want to cache it or not, which may be
useful in some cases.
msg52604 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2019-10-22.10:50:57
Here is the review.

The implementation does caching on the module level.
Some values are not cacheable. After fixing the tests I still wonder if a cache in our subclass of RawConfigParser is not more correct.
msg52603 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2019-10-22.10:39:02
Not a bug.
msg52601 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2019-10-22.10:31:44
Under some circumstances the calls to config.get or config.getint can become performance bottlenecks.

Those configuration values are static data and shouldn't vary between server restarts (even if we thought briefly about configuration hot reloading) and thus they should be cached.
History
Date User Action Args
2019-12-02 11:35:56roundup-botsetmessages: + msg53782
2019-12-02 11:35:53roundup-botsetstatus: testing -> resolved
messages: + msg53781
2019-11-29 12:05:10reviewbotsetmessages: + msg53732
2019-11-29 10:36:41reviewbotsetmessages: + msg53731
2019-11-29 10:30:30cedsetstatus: resolved -> testing
reviews: 284271002, 250131002 -> 284271002, 250131002,276511002
messages: + msg53730
assignedto: nicoe -> ced
2019-11-27 17:48:44roundup-botsetmessages: + msg53706
2019-11-27 17:48:38roundup-botsetmessages: + msg53705
2019-11-25 14:59:08roundup-botsetmessages: + msg53488
2019-11-25 14:59:04roundup-botsetstatus: testing -> resolved
nosy: + roundup-bot
messages: + msg53487
2019-11-18 18:06:59reviewbotsetmessages: + msg53329

Showing 10 items. Show all history (warning: this could be VERY long)