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

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.9.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/infinis... [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=a24cbadcbbe632f64eb44055... [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-update... [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