[PATCH] Configure.get cache

[mixedpuppy <shanec@…>]

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.

[Christian Boos]

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.

[mixedpuppy <shanec@…>]

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.

[Christian Boos]

Replying to mixedpuppy <shanec@…>:

2. 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…

[mixedpuppy <shanec@…>]

Replying to cboos:

Replying to mixedpuppy <shanec@…>:

2. 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.

[mixedpuppy <shanec@…>]

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

[Remy Blank]

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 ;-)

[mixedpuppy <shanec@…>]

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

[mixedpuppy <shanec@…>]

updated patch

[Christian Boos]

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.

[Remy Blank]

Postponing performance improvement after 0.12.

[Christian Boos]

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

[Christian Boos]

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.

[Christian Boos]

Add a cache in Configuration objects.

[Christian Boos]

Add a cache in Section objects.

[Remy Blank]

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

[Christian Boos]

Fine, second patch applied in r8727.