Uploaded image for project: 'Infinispan'
  1. Infinispan
  2. ISPN-2379

BoundedConcurrentHashMap improvements

This issue belongs to an archived project. You can view it, but you can't modify it. Learn more

XMLWordPrintable

      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

              vblagoje Vladimir Blagojevic (Inactive)
              manik_jira Manik Surtani (Inactive)
              Archiver:
              rhn-support-adongare Amol Dongare

                Created:
                Updated:
                Resolved:
                Archived: