Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

#8510 closed defect (fixed)

[PATCH] Configure.get cache

Reported by: mixedpuppy <shanec@…> Owned by: Christian Boos
Priority: normal Milestone: 0.11.6
Component: general Version: 0.11.5
Severity: normal Keywords: performance config
Cc: Branch:
Release Notes:
API Changes:

Description

During my profiling of trac, the top hit for trac itself (at one point) was Configure.get. I added some simple value caching and dropped 6-10ms per request on my laptop. My system is not a base trac, so it could be issues at a higher level with some plugin, but just looking at the code patch a lot happens to get a config variable. This is not fully tested yet.

Attachments (4)

trac-config-cache.patch (1.7 KB ) - added by mixedpuppy <shanec@…> 10 years ago.
config-cache.patch (1.5 KB ) - added by mixedpuppy <shanec@…> 10 years ago.
updated patch
t8510-Configuration-cache-r8720.diff (2.5 KB ) - added by Christian Boos 10 years ago.
Add a cache in Configuration objects.
t8510-Section-cache-r8720.diff (2.5 KB ) - added by Christian Boos 10 years ago.
Add a cache in Section objects.

Download all attachments as: .zip

Change History (17)

by mixedpuppy <shanec@…>, 10 years ago

Attachment: trac-config-cache.patch added

comment:1 by Christian Boos, 10 years ago

Milestone: 0.12

A few remarks so far:

  1. there's a semantic change here: the first call to get(name, default1) will store default1 in the cache if _get(name, default1) returns default1. A second call to get(name, default2) would now return default1 whereas before the patch it would have returned default2
  2. reset shouldn't be a slot
  3. speaking about __slots__, I'm not sure this makes a big saving anyway, and that class now holds 2 dicts, so it's no exactly lightweight… Maybe the cache itself could be moved at the level of the Configuration object, so we could have only one dict?

To expand on the last remark, we could have in Section:

    def get(self, name, default=''):
        key = (self.name, name)
        global default_marker
        value = self.config._cache.get(key, default_marker)
        if value is default_marker:
            value = self._get(name, default_marker)
            if value is not default_marker:
                self.config._cache[key] = value
        return value

default_marker is some global object used as a marker, used first to do only one lookup in the cache, then to avoid semantic change discussed in point 1. above.

default_marker = object()

Also, neither the above code (nor the proposed patch) is thread-safe, but it's probably not a problem as the only race I see could at worst lead to multiple calls to _get, storing multiple times the (same) value in the cache.

comment:2 by mixedpuppy <shanec@…>, 10 years ago

  1. right, that is fixable, I did something pretty close to what you suggested
  2. reset had to be in slots to call it from the config class

There are a few odd behaviors I'm running into when I try to move the cache into the configuration class, I'll see if I can resolve those.

in reply to:  2 ; comment:3 by Christian Boos, 10 years ago

Keywords: config added

Replying to mixedpuppy <shanec@…>:

  1. reset had to be in slots to call it from the config class

Sorry for being thick on this one, but why should there be a reset slot here, in order for the reset method to be callable? (and not for the other methods). Some tests on the interactive shell show this is not needed…

in reply to:  3 comment:4 by mixedpuppy <shanec@…>, 10 years ago

Replying to cboos:

Replying to mixedpuppy <shanec@…>:

  1. reset had to be in slots to call it from the config class

Sorry for being thick on this one, but why should there be a reset slot here, in order for the reset method to be callable? (and not for the other methods). Some tests on the interactive shell show this is not needed…

I haven't looked at why, and I don't get it either. Here's what I did:

  • added caching to section classes
  • loaded admin panels
  • changed something
  • change didn't stay
  • added section[name]._cache={} where reset is called now (config.save)
  • replayed above
  • change stayed
  • changed that to the reset call, replayed above
  • change didn't stay
  • added reset to slot, replayed
  • change stayed

It's likely something else and things working after adding reset to slot may have simply a coincidence.

comment:5 by mixedpuppy <shanec@…>, 10 years ago

/me thinks it would be nice to be able to edit ones own posts so I can fix formatting

in reply to:  5 comment:6 by Remy Blank, 10 years ago

Replying to mixedpuppy <shanec@…>:

/me thinks it would be nice to be able to edit ones own posts so I can fix formatting

Coming soon to a Trac instance near you: #454 ;-)

comment:7 by mixedpuppy <shanec@…>, 10 years ago

After figuring out the strange effect I was getting (re: reset in slot), dealing with default value properly, the net result was unchanged performance. Here's a dtrace output however, showing that the get is the hot spot from trac on a basic request. This is the exclusive timing and without any patch. It's what led me down this path seeing if there was an easy gain.

core.py        func   __new__             2712
text.py        func   to_unicode          3071
builder.py     func   _kwargs_to_attrs    3152
chrome.py      func   prepare_request     3175
api.py         func   __getattr__         3826
directives.py  func   __call__            3986
markup.py      func   _match              4144
path.py        func   _test               4157
config.py      func   __get__             4864
base.py        func   _eval_expr          4961
eval.py        func   evaluate            5616
base.py        func   _flatten           13471
config.py      func   get                17031
path.py        func   _multi             17072
markup.py      func   _strip             17106
output.py      func   __call__           29126
path.py        func   _generate          29568
output.py      func   encode             59191
-              total  -                 313773

by mixedpuppy <shanec@…>, 10 years ago

Attachment: config-cache.patch added

updated patch

comment:8 by Christian Boos, 10 years ago

About the last patch:

  1. the way you do get(), checking against the provided default, you won't cache the result of _get() if it returns a value equal to default even if that was the configured value; this is why I suggested the default_marker approach which can avoid this problem and hence would end up caching more things
  2. for the set(), self.config._cache.pop((self.name, name), None) would be enough

If even by fixing 1. we get no difference in performance, then I think it's not worth adding a cache.

comment:9 by Remy Blank, 10 years ago

Milestone: 0.12next-minor-0.12.x

Postponing performance improvement after 0.12.

comment:10 by Christian Boos, 10 years ago

Not to mention that the profiling of get should be redone after r8644:8645 (see TracDev/Performance#Profiling).

comment:11 by Christian Boos, 10 years ago

Milestone: next-minor-0.12.x0.11.6
Owner: set to Christian Boos
Status: newassigned

I'm attaching my versions of the patch, which indeed speed up things a tiny bit (not much, a few % on the average request). I tried both a cache at the level of Configuration and a cache in each Section, like originally suggested, and the lattert seems to be slightly faster, but it's hard to tell.

The main saving was done in r8644:8645, you shouldn't get such high counts of get calls anymore like in comment:7, unless your requesting a really costly page (e.g. I had 15000 hits when accessing my test environment's timeline with 1500 days back ;-) ). A typical page is now doing 200-500 calls to get.

As this is a speed improvement nevertheless, I'd like to commit it one of these two patches on 0.11.6.

by Christian Boos, 10 years ago

Add a cache in Configuration objects.

by Christian Boos, 10 years ago

Add a cache in Section objects.

comment:12 by Remy Blank, 10 years ago

Looks good. I find the patch in Section objects more readable as well.

comment:13 by Christian Boos, 10 years ago

Resolution: fixed
Status: assignedclosed

Fine, second patch applied in r8727.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.