Skip to content
This repository has been archived by the owner on Jan 9, 2018. It is now read-only.

TimeoutHashMap alternative #26

Open
ghetolay opened this issue Feb 14, 2014 · 6 comments
Open

TimeoutHashMap alternative #26

ghetolay opened this issue Feb 14, 2014 · 6 comments
Assignees
Milestone

Comments

@ghetolay
Copy link
Owner

I've merged all issue related to TimeoutHashMap here.
The idea is to use an alternative rather than "fixing" the actual implementation.

merged from #23 #24 #25

@ghetolay
Copy link
Owner Author

Before creating the TimeoutHashMap I found that Guava provide Cache and this could work.

Problem are :

  • timeout is map dependant not item dependant
  • we will have to add a whole dependency for just a tiny piece of it.

@trumpetinc
Copy link
Collaborator

Hah - I looked at Guava cache also (and several others)! I would prefer to create the new TimeoutHashMap implementation - it'll be a very simple, elegant class (much more performant than a general purpose cache class could achieve). Guava dependency is OK for end-user, but I would prefer to avoid that size of dependency unless we have absolutely no choice. I'll put something together (hopefully this weekend).

@trumpetinc
Copy link
Collaborator

I've started putting together the alternative implementation of TimeoutHashMap, and I have a question:

In DefaultRPCSender it looks for all the world like we are never removing result listeners from the timeout map. We put result listeners in, and we look them up, but we never remove them after the call is complete.

Am I just totally missing something? Or was the old behavior just always relying on the timeouts?

If I'm thinking about it clearly, onCallResult shoulud remove the listener from the map, right?

@trumpetinc
Copy link
Collaborator

ok - I have comitted a new branch with a new implementation of TimeOutHashMap (and changing DefaultRPCSender so it uses remove() instead of get() when a response is received). https://github.com/ghetolay/jwamp/tree/timeouthashmap-redesign

I look forward to your review!

@ghetolay
Copy link
Owner Author

Yes absolutely it should be remove() instead of get(). Also containsKey() should not be use cause it's kind of useless when you don't use null key.

I may have done something wrong with git because I'm really surprised of that.

@trumpetinc
Copy link
Collaborator

I just finished up a few finishing touches on the new TimeOutHashMap. I'm wondering if we should rename this to TimeoutCache or something like that - it doesn't fully implement the Map interface (and honestly, I'm not sure that it's worth doing so - it would be a lot of work, and we really don't need the additional Map functionality).

Anyway, let me know what you think about the code. We could probably be a bit more judicious with when the pollTimeouts method gets called by deciding that the timeout contract is a loose guideline for eviction instead of a hard requirement. For example, we could just call it on put() - but for now, that is feeling like a micro-optimization that can be done if future profiling shows a bottleneck.

@ghetolay ghetolay added this to the JSR356 milestone Feb 19, 2014
ghetolay added a commit that referenced this issue Mar 3, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants