On Thu, 4 Feb 2021 20:45:55 GMT, djelinski <github.com+30433125+djelin...@openjdk.org> wrote:
> Thanks for your review! Some comments below. > > > A full handshake or OCSP status grabbing could counteract all the > > performance gain with the cache update. > > Yes, but that's unlikely. Note that K=3 is before K=1 in the queue only > because 3 wasn't used since 1 was last used. This means that either K=3 is > used less frequently than K=1, or that all cached items are in active use. In > the former case we don't lose much by dropping K=3 (granted, there's nothing > to offset that). In the latter we are dealing with full cache at all times, > which means that most `put()`s would scan the queue, and we will gain a lot > by finishing faster. I may think it differently. It may be hard to know the future frequency of an cached item based on the past behaviors. For the above case, I'm not sure that K=3 is used less frequently than K=1. Maybe, next few seconds, K=1 could be more frequently. I would like a solution to following the timeout specification: keep the newer items if possible. > > > get() [..] without [..] change the order of the queue > > If we do that, frequently used entries will be evicted at the same age as > never used ones. This means we will have to recompute (full handshake/fresh > OCSP) both the frequently used and the infrequently used entries. It's better > to recompute only the infrequently used ones, and reuse the frequently used > as long as possible - we will do less work that way. > That's probably the reason why a `LinkedHashMap` with `accessOrder=true` was > chosen as the backing store implementation originally. > See above. It may be true for some case to determine the frequency, but Cache is a general class and we may want to be more careful about if we are really be able to determine the frequency within the Cache implementation. > > get() performance is more important [..] so we should try to keep the cache > > small if possible > > I don't see the link; could you explain? > link? Did you mean the link to get() method? It is a method in the Cache class. > > In the update, the SoftReference.clear() get removed. I'm not sure of the > > impact of the enqueued objects any longer. In theory, it could improve the > > memory use, which could counteract the performance gain in some situation. > > That's the best part: no objects ever get enqueued! We only called `clear()` > right before losing the last reference to `SoftCacheEntry` (which is the > `SoftReference`). When GC collects the `SoftReference`, it does not enqueue > anything. GC only enqueues the `SoftReference` when it collects the > referenced object (session / OCSP response) without collecting the > `SoftReference` (cache entry) itself. > This is [documented > behavior](https://docs.oracle.com/javase/7/docs/api/java/lang/ref/package-summary.html): > _If a registered reference becomes unreachable itself, then it will never be > enqueued._ > I need more time for this section. > > Could you edit the bug > > I'd need an account on the bug tracker first. Okay. No worries, I will help you if we could get an agreement about the update. ------------- PR: https://git.openjdk.java.net/jdk/pull/2255
