BoundedConcurrentHashMap improvements

Manik initiated conversation about possible improvements to locking mechanism in BoundedConcurrentHashMap(BCHM) as well as few other minor improvements for BCHM.

galderz: vblagoje, manik ping\
[12:13pm] vblagoje: hey galderz
[12:13pm] galderz: hi guys, just finished reading the emails re: segment locking
[12:14pm] vblagoje: yes, I'll come back to it as soon as I finish some other cancellation stuff I am working on
[12:14pm] galderz: vblagoje, what was the reason for doing eviction on the user's thread?
[12:14pm] vblagoje: but what manik said makes sense galderz
[12:15pm] galderz: vblagoje, hmmmm, how long would that be? this is important stuff for on-going perf tests that eap guys are doing...
[12:15pm] vblagoje: galderz IIRC that is how they do it in paper we took these ideas from
[12:15pm] vblagoje: aha got it, so this is top prio, was not aware of it
[12:16pm] manik: galderz: evicton on the user thread is not the problem
[12:16pm] galderz: manik, well, the lock
[12:16pm] manik: galderz, vblagoje: the problem is in our implementation of it, where all threads queue up and try and do it
[12:16pm] manik: (i.e., all threads wait for that lock)
[12:16pm] manik: only one should do that
[12:16pm] vblagoje: yes manik you are quite right
[12:16pm] galderz: manik, but if one does it, what do the others do?
[12:17pm] manik: galderz: I don't know, some real user work, perhaps?
[12:17pm] galderz: manik, lol
[12:17pm] manik: such as serving up porn webpages?
[12:17pm] galderz: manik, i mean: the collection will be modified, so wondering if the users can still query the segment and get a correct view of it...
[12:18pm] galderz: manik, one option i had in mind is to do eviction in a separate thread, but it requires that there's a separate thread
[12:18pm] galderz: though i guess the locking issue would still be there
[12:18pm] galderz: actually, forget what I said...
[12:19pm] vblagoje: i thought we had background worker thread that kicks in as well - I forgot - we've done this long time ago
[12:19pm] manik: galderz: we moved away from the separate thread design many versions ago, we're not going back
[12:20pm] manik: vblagoje: no we dont
[12:20pm] galderz: manik, i agree, plus it does not help anyway
[12:20pm] manik: vblagoje: that's just for expiration
[12:20pm] vblagoje: right manik
[12:20pm] manik: vblagoje, galderz: I think the correct solution is to simply work on the tryLock() that we already have
[12:20pm] manik: vblagoje, galderz: one thread is bound to win, and perform the eviction.
[12:20pm] vblagoje: yeah manik, I think so too - there might be a better solution there
[12:20pm] manik: vblagoje, galderz: the others won't , and will proceed with whatever they need to do
[12:21pm] emmanuel left the chat room. (Quit: Leaving.)
[12:21pm] galderz: manik, right, and the others will skip it?
[12:21pm] manik: galderz: yup
[12:21pm] galderz: manik, sounds good
[12:21pm] galderz: vblagoje, i can implement this if you're busy
[12:21pm] galderz: manik, ^
[12:21pm] galderz: or give it a shot
[12:21pm] vblagoje: sure give it a shot
[12:21pm] galderz: since i'm doing all the perf testing and profiling....
[12:21pm] vblagoje: it is a very delicate algorithm
[12:21pm] manik: but before we do that, I just want to know why we have the additional block where we do the lock() if tryLock() fails. vblagoje?
[12:22pm] manik: galderz, vblagoje ^^
[12:22pm] vblagoje: one sec, lemmesee
[12:23pm] galderz: might be stuff trustin added...
[12:23pm] vblagoje: well I think this is just the way for other non-winning threads to wait
[12:23pm] galderz: ISPN-719 ?
[12:23pm] jbossbot: jira ISPN-719 BoundedConcurrentHashMap.EvictionListener should have a bulk entry listener method. [Resolved (Done) Task, Major, Trustin Lee] https://issues.jboss.org/browse/ISPN-719
[12:23pm] vblagoje: no no wait
[12:23pm] vblagoje: there was a reason
[12:24pm] galderz: actually, that was already before:
[12:24pm] galderz: https://source.jboss.org/browse/Infinispan/core/src/main/java/org/infinispan/util/concurrent/BoundedConcurrentHashMap.java?r2=72f06936c1338cb92825b65bc466a2faa7b8ed73&r1=16ec9a8ccfa75c8a8231b71042c0fea293322774
[12:25pm] Kosch left the chat room. (Remote host closed the connection)
[12:25pm] vblagoje: yes yes it was there before, for a reason, I have to dig up my notes
[12:25pm] galderz: yeaj:
[12:25pm] galderz: https://source.jboss.org/changelog/Infinispan?cs=a24cbadcbbe632f64eb440552b8f90ab41811ecc
[12:26pm] navssurtani left the chat room. (Quit: Leaving)
[12:26pm] galderz: brb
[12:27pm] vblagoje: haha you crazy man, I think this is a better resource http://infinispan.blogspot.ca/2010/03/infinispan-eviction-batching-updates.html
[12:28pm] manik: vblagoje: I still don't see why you need the hard lock() call?
[12:30pm] vblagoje: the notes say manik that " In case when thread's queue is totally full a lock must be explicitly requested"
[12:31pm] manik: vblagoje: Thread's queue? The queue is not per thread, is it?
[12:32pm] vblagoje: no manik, it so for all threads
[12:32pm] vblagoje: how would you do it with only tryLock
[12:32pm] manik: vblagoje: right, so only one thread should attempt the lock
[12:32pm] vblagoje: right manik
[12:32pm] manik: the tryLock will only fail of someone already holds the lock
[12:33pm] manik: and will succeed if no one holds the lock
[12:33pm] manik: ergo, one thread will "win", the others will "lose".
[12:33pm] manik: (win, meaning, win the privilege to carry out the eviction )
[12:33pm] vblagoje: yeah, you are right
[12:33pm] vblagoje: let me rewrite this
[12:34pm] manik: Nothing to rewrite, just 4 lines of code to comment out
[12:34pm] manik: vblagoje: I think we just need to comment out lines 1727 to 1730
[12:34pm] manik: galderz: ^^
[12:35pm] vblagoje: yes, seems like it manik
[12:35pm] manik: galderz, vblagoje: let me send you guys a patch…
[12:35pm] manik: along with some other minor improvements too
[12:35pm] manik: vblagoje, galderz: and I'd like your feedback on it
[12:36pm] vblagoje: right right, and we have eviction.thresholdExpired on 1733 - so all good!
[12:36pm] vblagoje: I like it manik actually
[12:36pm] vblagoje: much much better
[12:40pm] galderz: back now
[12:40pm] galderz: manik, are you sending a pull req?
[12:42pm] vblagoje: i think he is galderz
[12:42pm] galderz: vblagoje, ok
[12:43pm] galderz: i'll focus on the 2LC changes that i have queued...
[12:43pm] alesj left the chat room. (Quit: Leaving.)
[12:44pm] salaboy joined the chat room.
[12:44pm] salaboy left the chat room. (Changing host)
[12:44pm] salaboy joined the chat room.
[12:49pm] asantos_ joined the chat room.
[1:01pm] manik: galderz: yes, coming up now
[1:02pm] galderz: manik, no worries - i'm working on the 2LC code
[1:02pm] galderz: manik, and taking the opportunity to get rid of some useless code… incompatible encoding
[1:02pm] galderz: less intermediate objects
[1:02pm] manik: galderz:
[1:03pm] manik: galderz, vblagoje: https://github.com/infinispan/infinispan/pull/1374
[1:03pm] manik: pls have a look
[1:03pm] vblagoje: cool manik, will do so
[1:03pm] hagarth left the chat room. (Ping timeout: 245 seconds)
[1:05pm] galderz: manik, looks good to me
[1:05pm] galderz: vblagoje, could you handle the pull req?
[1:05pm] vblagoje: sure galderz
[1:05pm] galderz: vblagoje, making sure the testsuite looks fine
[1:05pm] galderz: vblagoje, thx
[1:05pm] galderz: we're gonna kick ass...
[1:07pm] gscheibel left the chat room. (Quit: On the other hand, you have different fingers.)
[1:09pm] vblagoje: ok galderz, will do
[1:09pm] manik: vblagoje, galderz: keep me posted
[1:21pm] vblagoje: manik, mind if I take over this PR, I think I need to make slight adjustments - i think evicted Set should be non null but empty Set by contract
[1:21pm] vblagoje: I'll create JIRA as well