Infinispan
  1. Infinispan
  2. ISPN-2379

BoundedConcurrentHashMap improvements

    Details

    • Similar Issues:
      Show 10 results 

      Description

      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

        Activity

        Hide
        Galder Zamarreño
        added a comment -

        This needs to be fixed in 5.1.x as well.

        Show
        Galder Zamarreño
        added a comment - This needs to be fixed in 5.1.x as well.
        Hide
        Adrian Nistor
        added a comment -

        Could you please explain in a few words what is this all about?

        Show
        Adrian Nistor
        added a comment - Could you please explain in a few words what is this all about?
        Hide
        Dan Berindei
        added a comment -

        Adrian Nistor Just read the patch, it's a lot easier on the eyes

        Show
        Dan Berindei
        added a comment - Adrian Nistor Just read the patch, it's a lot easier on the eyes
        Hide
        Vladimir Blagojevic
        added a comment -

        Hahaha, very good! The main change is in attemptEviction method of Segment!

        Show
        Vladimir Blagojevic
        added a comment - Hahaha, very good! The main change is in attemptEviction method of Segment!

          People

          • Assignee:
            Vladimir Blagojevic
            Reporter:
            Manik Surtani
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: