Change object caches from Queue to Deque or List

[zzz]

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.

[zab]

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 ​https://github.com/zlatinb/atomics/tree/master/src/zab/atomics/pool </shameless self-promotion>

[zab]

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

[zzz]

• TryCache? is straightforward and does what we want (LIFO and uses tryLock())

• I'd prefer naming the methods simply acquire() and release()

• We would probably want to use this in core (e.g. in ByteCache? and SimpleByteCache?) so we'd need to move it to net.i2p.util

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

[zab]

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.

[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

[zzz]

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

[zzz]

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

[zzz]

re: comment 1 (single ReentrantLock?)
LBQ does have two ReentrantLocks? with a Condition for each
​http://kickjava.com/src/java/util/concurrent/LinkedBlockingQueue.java.htm lines 85-95