Looks fine to me.

Xuelei

On 2/23/2018 6:13 AM, Weijun Wang wrote:
Updated at http://cr.openjdk.java.net/~weijun/8197518/webrev.02/.

1. ConcurrentHashMap used in MemoryCache. No more "content.remove(key)" but 
it's actually useless because rc.isEmpty() will not be true normally. I'll think about 
how to clean up unused AuthList objects in another RFE.

2. AuthList now removes entries from tail backwards until one is within 
lifespan. I still kept an oldestTime field. In my experiment it is useful but a 
bigger value does not help much, so I just hardcoded it to 5 seconds.

Thanks
Max

On Feb 23, 2018, at 4:57 PM, Weijun Wang <weijun.w...@oracle.com> wrote:

Brilliant idea. I will do more experiment.

Thanks
Max

On Feb 23, 2018, at 11:25 AM, Xuelei Fan <xuelei....@oracle.com> wrote:

Hi Weijun,

I thought more about the performance impact.  The impact may mainly come from 
the big size of the cached entries.

The current implementation needs to go over the full list: find the 1st expired 
item and then remove the rest.  The previous one have an additional round with 
entries.indexOf().  Could we just start from the end of the list?

   while (true) {
      E e = entries.removeLast();
      if e not expired {
         add it back and break;
      }
   };

If the list is really big, and the lifetime is significant big as well (>> 1 minute), 
iterate from the oldest item (backward from the end of the list) may be much more effective.  
LinkedList itself is not synchronized, so as if there is not too much items to go over, the 
performance should be fine.  I'm hesitate to hard code a cleanup every 1 minute strategy.  If 
we clean often, there may be not too much items to go over in the list.  So we might be able to 
remove the "at most every minute" strategy.

Xuelei

On 2/22/2018 5:55 PM, Weijun Wang wrote:
Updated webrev at http://cr.openjdk.java.net/~weijun/8197518/webrev.01/.
On Feb 23, 2018, at 9:02 AM, Weijun Wang <weijun.w...@oracle.com> wrote:

You mean I can save it somewhere and only update it when a cleanup is performed?

This should be better. Now there will be only isEmpty(), getFirst() and 
addFirst(), and one less getLast().

Thanks
Max

On Feb 23, 2018, at 1:45 AM, Xuelei Fan <xuelei....@oracle.com> wrote:

Looks like list synchronization is a factor of the performance impact. Maybe, you can 
have a private time for the oldest entry so don't access/iterate/cleanup entries list 
until necessary.  The "at most every minute" may be not a good strategy in some 
situations.
In fact, it's now almost "exactly every minute". What situations do you think 
it's not good? I cannot use size() because I have to remember all entries with lifespan 
to be correct.
Thanks
Max

Xuelei

On 2/22/2018 12:36 AM, Weijun Wang wrote:
Please take a review at
  http://cr.openjdk.java.net/~weijun/8197518/webrev.00/
Two notes:
1. I tried list.subList(here, end).clear() but it's not faster.
2. I have looked at ConcurrentHashMap + ConcurrentSkipListMap but will need 
more time to verify its correctness and measure the performance gain. Since the 
bug is reported on 8u, a safer fix looks better.
Noreg-perf.
Thanks
Max



Reply via email to