[
https://issues.jboss.org/browse/ISPN-2379?page=com.atlassian.jira.plugin....
]
Vladimir Blagojevic updated ISPN-2379:
--------------------------------------
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/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
BoundedConcurrentHashMap improvements
-------------------------------------
Key: ISPN-2379
URL:
https://issues.jboss.org/browse/ISPN-2379
Project: Infinispan
Issue Type: Enhancement
Components: Eviction
Affects Versions: 5.2.0.Beta1
Reporter: Manik Surtani
Assignee: Vladimir Blagojevic
Fix For: 5.2.0.CR1
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
--
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