Opened 3 years ago

Last modified 3 years ago

#2263 accepted enhancement

Change object caches from Queue to Deque or List

Reported by: zzz Owned by: zzz
Priority: minor Milestone: 0.9.36
Component: api/general Version: 0.9.34
Keywords: Cc: Zlatin Balevsky
Parent Tickets: Sensitive: no


ref: #2247

In a number of places we cache objects with a LinkedBlockingQueue? (LBQ). Original reason was to take advantage of separate locking for read and write sides. True?

LBQ has two downsides - additional object churn as each object must be wrapped, and less CPU cache hit ratio because it's not LIFO. Investigate and consider switching to Deque (still wrapped) or List (not). Either goes back to a single lock. Decision may be different for different use cases. A general helper class may also be useful.


Change History (8)

comment:1 Changed 3 years ago by Zlatin Balevsky

LBQ has a single ReentrantLock object but two Condition objects - one for "notFull" and one for "notEmpty" conditions. So it is no different from a single lock approach (except for situations with multiple producers and consumers, but we're not targeting those).

The approach I used in #2247 has the benefit that we have the option of using "tryLock" which is appropriate for some time-critical threads.

The ultimate solution I think is a FreeList approach like </shameless self-promotion>

comment:2 Changed 3 years ago by Zlatin Balevsky

I implemented a tryLock()-based cache on branch "i2p.i2p.zab.2263" and converted ntcp and ssu code to use it.

comment:3 Changed 3 years ago by zzz

  • TryCache? is straightforward and does what we want (LIFO and uses tryLock())
  • I'd prefer naming the methods simply acquire() and release()
  • In EventPumper?.releaseBuf() you removed the size check and buf.clear(), those appear to be important
  • In UDPPacket, the static RouterContext? could be a problem in MultiRouter? or Android. Maybe the cache needs to be hung off the transport or something. That's always been a little messy there. Ugly that context isn't final.

comment:4 Changed 3 years ago by Zlatin Balevsky

Pushed some changes on the branch which address most of the comments:

  • changes method names to acquire() and release()
  • moves TryCache to net.i2p.util in core
  • clear()s the buffer before returning to cache, that was a good catch! The size check only provides a false sense of security so kept that one out
  • UDPPacket - the simplest thing that I could think of is to null the context reference when clear()-ing the cache, which only happens on shutdown and is done to prevent context references on Android. So that should fix Android, no idea about MultiRouter. But to properly fix this would be a much more invasive change. I don't mind doing it, if that's what you want.

comment:5 Changed 3 years ago by zzz

re: invasive change, not for now, I need to look at it again

re: ByteCache? and SimpleByteCache?, may have spoken too soon, not sure if either can actually extend TryCache?<ByteArray? or byte[]> due to other stuff they are doing, we may just have to use the techniques rather than strictly extending it. But those aren't the only possibilities in core.

Re: false sense of security? Seems real to me. We can talk about it today

comment:6 Changed 3 years ago by zzz

Milestone: undecided0.9.36
Owner: set to zzz
Status: newaccepted

Branch propped in 18b3ff2c5798fc0986658c028ba37137fd0ef34a to be 0.9.35-7
I'll do the ByteCache? and SimpleByteCache? changes to use TryCache?, to follow shortly.

comment:7 Changed 3 years ago by zzz

ByteCache? and SimpleByteCache? in f1393865fcc91def6c792b00b4d5f621404e1c2f 0.9.35-10
Leaving open for testing and looking for any more candidates

comment:8 Changed 3 years ago by zzz

re: comment 1 (single ReentrantLock?)
LBQ does have two ReentrantLocks? with a Condition for each lines 85-95

Note: See TracTickets for help on using tickets.