Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lock using the cache key instead of a global lock #3

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

danomatic
Copy link

Lock using the cache key instead of a global lock, which allows more concurrency without race conditions.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.258% when pulling dda06c7 on danomatic:key-based-lock into 99ea324 on arhs:master.

@dnlprplt
Copy link
Contributor

Can you point to any evidence that String.intern() can be used for locking?

There are lots of contradicting posts on the subject. Here for example is stated that it's a bad practice: https://stackoverflow.com/questions/133988/problem-with-synchronizing-on-string-objects

Other suggest using a mutex, but wether this is worth it is debatable.

@danomatic
Copy link
Author

My rough understanding is that it is safe and efficient, especially for Java 7 and 8. Some details here:
http://java-performance.info/string-intern-in-java-6-7-8/

But don't take my word for it. Synchronizing all cache access across all threads was bad for our application, especially because we have slow API calls inside the @Cacheable method.

@dnlprplt
Copy link
Contributor

I've been investigating a little and apart from the article you reference, I haven't found any other evidence that suggests we can safely lock on String.intern. Also the article is not explicitly stating we can lock on it.

I've found another solution though that I think can suit our need. It has been suggested (here https://stackoverflow.com/questions/11124539/how-to-acquire-a-lock-by-a-key) to use the Striped class from Guava. The javadoc on the class explains it all: https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/Striped.java

If you rewrite the code to use the Striped class, I will accept the PR.

@danomatic
Copy link
Author

seems too bad to pull in all of guava for that, but it does look nice.

@danomatic
Copy link
Author

danomatic commented Sep 16, 2017

What about this lighter solution from the same SO post? It seems very similar.
https://stackoverflow.com/a/36054928/2020766

@dnlprplt
Copy link
Contributor

Yes, it is based on the same idea. We can go for it as I also prefer keeping the library light.

We should decide on a default value for the size. And make it configurable.

@cyrilschumacher cyrilschumacher merged commit 6c2051f into arhs:master Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants