Skip to content

Conversation

@turbanoff
Copy link
Contributor

SoftReference.get() is not supposed to be called more than once. It's because between two .get() calls GC could clear the reference and code could get an expected result.
In AspectJ it called twice in a few places, which we could easily update.

@kriegaex
Copy link
Contributor

kriegaex commented Apr 15, 2022

SoftReference.get() is not supposed to be called more than once.

Says who? Can you elaborate, please? I am not an expert on the subject matter, but I would rather think that get() should be called every time we want to use the referent, because precisely that could have happened: It was GC-ed in the meantime. The whole idea of a soft reference is to not use the referent anymore if it was GC-ed. By creating a strong reference and returning it, no matter what, we effectively block GC at the last second.

If your concern here is rather that the methods in question would return null in case GC happened before the second get() and that could lead to unexpected side effects, then I understand better. But that should not be a problem, because if get() yields null at the first call, we also return null. The only difference is that if the second call is null, we do not remove the map entry. But that would happen when it is requested the next time.

@kriegaex
Copy link
Contributor

@turbanoff, I know you seem to avoid writing a lot, which can be a nice quality sometimes. OTOH, you keep opening new PRs without responding to feedback in existing ones first. In order to keep the number of open PRs manageable, it would be nice to have a short cycle time for them. In the next few months I will have even less time than before due to my work to click through open issues. I also want to avoid letting your very welcome contributions rot. So please respond. Thank you so much.

@turbanoff
Copy link
Contributor Author

turbanoff commented Apr 21, 2022

By creating a strong reference and returning it, no matter what, we effectively block GC at the last second.

Actually, for JVM, even get() call creates a strong reference. It doesn't matter if code assigns it to variable or not. It was one of the reason, why new method Reference.refersTo was added in Java 16 (JDK-8188055).

I agree, that there are no real problems here in updated code. Let me update it to make it cleanup only in obvious cases (in remove)

@turbanoff turbanoff force-pushed the call_SoftReference.get_only_once branch from 03256a7 to 7c5de4a Compare April 21, 2022 06:52
@jafarre-bi
Copy link

Agreed @turbanoff . Wherever one can use refersTo it's always a better alternative to get(), unless you precisely intend to trigger the side effects of get(). If strong references are created too often, it may be impossible for the GC ever to clear them. With SoftReferences it's even worse, because they keep an internal timestamp to implement some sort of LRU policy, and that timestamp gets updated every time you call get().
But regardless of that, it's clear that the three ifs you removed were completely unnecessary.

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.

3 participants