[infinispan-issues] [JBoss JIRA] (ISPN-2379) BoundedConcurrentHashMap improvements

[Galder Zamarreño (JIRA)]

     [ https://issues.jboss.org/browse/ISPN-2379?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Galder Zamarreño updated ISPN-2379:
-----------------------------------

           Status: Resolved  (was: Pull Request Sent)
    Fix Version/s: 5.1.8.Final
                       (was: 5.1.9.Final)
       Resolution: Done

    
> BoundedConcurrentHashMap improvements
> -------------------------------------
>
>                 Key: ISPN-2379
>                 URL: https://issues.jboss.org/browse/ISPN-2379
>             Project: Infinispan
>          Issue Type: Enhancement
>          Components: Eviction
>    Affects Versions: 5.1.7.Final, 5.2.0.Beta1
>            Reporter: Manik Surtani
>            Assignee: Vladimir Blagojevic
>              Labels: jdg, jdg6
>             Fix For: 5.2.0.CR1, 5.1.8.Final
>
>
> 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

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira