Cache Query Fields in core requests #13248
Conversation
Fix #13242 by caching query fields in core requests
| @Override | ||
| public Object setAttribute(String name, Object attribute) | ||
| { | ||
| // Short cut for known common implementation attributes to avoid the creation of a lazy attribute map. | ||
| // These attributes are hidden from name set | ||
| return switch (name) | ||
| { | ||
| case CachedQueryFields.KEY -> | ||
| { | ||
| CachedQueryFields cqf = _cachedQueryFields; | ||
| _cachedQueryFields = attribute instanceof CachedQueryFields fields ? fields : null; | ||
| yield cqf; | ||
| } | ||
| case COOKIE_ATTRIBUTE -> | ||
| { | ||
| CookieCache old = _cookieCache; | ||
| _cookieCache = attribute instanceof CookieCache cookieCache ? cookieCache : null; | ||
| yield old; | ||
| } | ||
| default -> super.setAttribute(name, attribute); | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public Object getAttribute(String name) | ||
| { | ||
| return switch (name) | ||
| { | ||
| case CachedQueryFields.KEY -> _cachedQueryFields; | ||
| case COOKIE_ATTRIBUTE -> _cookieCache; | ||
| default -> super.getAttribute(name); | ||
| }; | ||
| } |
There was a problem hiding this comment.
@lorban @sbordet This is probably a second order optimisation that need not be in this PR, but I thought I'd raise the question.
We now have a few request attributes used for caching derived data. This means that very few requests will actually not have attributes. So we probably should either just remove the Lazy attribute implementation or make an effort like this so that common attributes will not force an attribute creation.
I don't think this is so much about speed of lookup, but more about allocation of a map of attributes per request. But looking at ConcurrentHashMap, it looks very cheap to create an empty one, so I'm not sure lazy attributes ios worthwhile at all.
Perhaps, this PR should just be about the CachedQueryFields and we open another PR/Issue to examine if Lazy attributes are needed and if this kind of "optimization" is worthwhile
|
@lachlan-roberts @sbordet @lorban This is an almost trivial PR... except it does raise the question about do we need Lazy attributes anymore and/or if optimizations like this PR attempts are worthwhile to avoid creating the lazy attribute map. |
|
Wouldn't it be better to cache them in the HttpURI object itself? |
Interesting idea.... investigating.... |
@joakime it kind of works, but kind of doesn't. The cached Fields are not just related to the |
Fix #13242 by caching query fields in core requests