-
1. Re: Performance Improvement Proposal: Cache result of HttpSe
dward Dec 7, 2009 9:49 AM (in response to dward)The Subject of this post got chopped. It was supposed to read, "Performance Improvement Proposal: Cache result of HttpServletRequest.getLocalName() in HttpMessageComposer"
-
2. Re: Performance Improvement Proposal: Cache result of HttpSe
kconner Dec 7, 2009 10:13 AM (in response to dward)Good catch David.
I have created https://jira.jboss.org/jira/browse/JBESB-3043 and set the fix revision to 4.7 CP1. Could you commit this into the CP branch?
I'm about to create a tag on that branch, so hold off for a while. Will ping you when I am done.
Thanks
Kev -
3. Re: Performance Improvement Proposal: Cache result of HttpSe
kconner Dec 7, 2009 10:14 AM (in response to dward)The code as posted in not thread safe though, could you fix that up at the same time?
Thanks,
Kev -
4. Re: Performance Improvement Proposal: Cache result of HttpSe
dward Dec 7, 2009 10:31 AM (in response to dward)Granted the code as posted leaves the door open for the possibility for a call to request.getLocalName() to be called and cached more than once, but this would only happen very early on in the server instance's life. I had to weigh that vs. having to synchronize getting the localName from the localAddr_to_localName map every invocation... Thoughts?
-
5. Re: Performance Improvement Proposal: Cache result of HttpSe
kconner Dec 7, 2009 10:33 AM (in response to dward)The bug is in the use of hashmap outside of the synchronisation, as that fails in certain memory models. Caching multiple times is not the issue, although though could theoretically happen.
Kev -
6. Re: Performance Improvement Proposal: Cache result of HttpSe
dward Dec 7, 2009 10:42 AM (in response to dward)Okay I'll include the get in the synchronized block then.
-
7. Re: Performance Improvement Proposal: Cache result of HttpSe
kconner Dec 7, 2009 10:45 AM (in response to dward)Or use the concurrent one.
-
8. Re: Performance Improvement Proposal: Cache result of HttpSe
dward Dec 7, 2009 10:55 AM (in response to dward)Again, something to weigh. If I do this:
private static final String getLocalName(HttpServletRequest request) { String localAddr = request.getLocalAddr(); String localName = localAddr_to_localName.get(localAddr); if (localName == null) { localName = request.getLocalName(); localAddr_to_localName.put(localAddr, localName); } return localName; } private static final Map<String,String> localAddr_to_localName = Collections.synchronizedMap(new HashMap<String,String>());
The it is thread safe, but opens the door for multiple calls to request.getLocalName(). But if I do this:private static final String getLocalName(HttpServletRequest request) { String localAddr = request.getLocalAddr(); synchronized (localAddr_to_localName) { String localName = localAddr_to_localName.get(localAddr); if (localName == null) { localName = request.getLocalName(); localAddr_to_localName.put(localAddr, localName); } return localName; } } private static final Map<String,String> localAddr_to_localName = new HashMap<String,String>();
Then request.getLocalName() will only ever get called once, since both the get and put calls to the map are in the same atomic block. -
9. Re: Performance Improvement Proposal: Cache result of HttpSe
kconner Dec 7, 2009 10:57 AM (in response to dward)How about just using ConcurrentHashMap??
-
10. Re: Performance Improvement Proposal: Cache result of HttpSe
dward Dec 7, 2009 6:33 PM (in response to dward)I don't see how using ConcurrentHashMap would be any different than Collections.synchronizedMap(new HashMap()). So it would still open the door for multiple calls to request.getLocalName(). I also don't want to use oswego classes if not necessary.
-
11. Re: Performance Improvement Proposal: Cache result of HttpSe
bill.burke Dec 7, 2009 6:37 PM (in response to dward)"Kevin.Conner@jboss.com" wrote:
The bug is in the use of hashmap outside of the synchronisation, as that fails in certain memory models. Caching multiple times is not the issue, although though could theoretically happen.
Kev
FYI, doublecheck works:
http://bill.burkecentral.com/2008/10/30/double-check-does-work/
You have to be careful how you init and declare variables though. -
12. Re: Performance Improvement Proposal: Cache result of HttpSe
dward Dec 7, 2009 6:50 PM (in response to dward)That's good to know Bill, thanks. The difference with what I'm doing is that it's not a member variable (the map) that is being set lazily, but a populated entry in the map, so we just have to be careful against ConcurrentModificationExceptions.
Kev, if you're okay with me not using ConcurrentHashMap (and instead using synchronized as I elaborated on above), let me know. Either way, I'll wait for your word before I commit to the performance workspace and 4.7 CP branch. Thanks. -
13. Re: Performance Improvement Proposal: Cache result of HttpSe
kconner Dec 8, 2009 4:52 AM (in response to dward) -
14. Re: Performance Improvement Proposal: Cache result of HttpSe
kconner Dec 8, 2009 4:57 AM (in response to dward)"dward" wrote:
That's good to know Bill, thanks. The difference with what I'm doing is that it's not a member variable (the map) that is being set lazily, but a populated entry in the map, so we just have to be careful against ConcurrentModificationExceptions.
That is certainly one issue but nothing to do with the memory model."dward" wrote:
Kev, if you're okay with me not using ConcurrentHashMap (and instead using synchronized as I elaborated on above), let me know. Either way, I'll wait for your word before I commit to the performance workspace and 4.7 CP branch. Thanks.
Your code above is not thread safe, please do not commit that. Double checking is not where it is falls down, rather it is the updates to the structure of the map that would not be propagated correctly in certain memory models.
The only reason Bill's example works is because of the volatile keyword, as that now has the same semantics as synchronized, wrt memory.
Kev