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

Removed 'backed' last cache hit hard references; it seems that that i… #1645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vruano
Copy link
Contributor

@vruano vruano commented Jan 11, 2023

Description

Removed 'backed' last cache hit hard references; it seems that that it might be trying to solve an non-existent issue.
Changed WeakReference to SoftReference to make cache more persistent in memory. Addresses GATK issue #1643.

Things to think about before submitting:

  • [X, but not new tests] Make sure your changes compile and new tests pass locally.
  • Add new tests or update existing ones:
    • A bug fix should include a test that previously would have failed and passes now.
    • New features should come with new tests that exercise and validate the new functionality.
  • [NA] Extended the README / documentation, if necessary
  • Check your code style.
  • Write a clear commit title and message
    • The commit message should describe what changed and is targeted at htsjdk developers
    • Breaking changes should be mentioned in the commit message.
…t might be trying to solve an non-existent issue.

Chaned WeakReference to SoftReference to make cache more persistent in memory.
Addresses issue #1643.
@vruano vruano requested a review from droazen January 11, 2023 04:09
@lbergelson lbergelson requested a review from cmnbroad January 11, 2023 16:43
@lbergelson
Copy link
Member

@cmnbroad Can you take a look at this? It seems like weak -> soft reference is the right change, and hopefully removing the unsychronized method helps, but it would be good if you could take a look.

@lbergelson lbergelson removed the request for review from droazen January 30, 2023 20:31
@@ -116,7 +110,7 @@ else if (Defaults.USE_CRAM_REF_DOWNLOAD) {
}

private byte[] findInCache(final String name) {
final WeakReference<byte[]> weakReference = cacheW.get(name);
final SoftReference<byte[]> weakReference = cache.get(name);
if (weakReference != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explicitly check for an empty array here as well, and throw an informative error if you encounter one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances the bases array would be an empty array as supposed to a null (claimed by the GC)?

@cmnbroad
Copy link
Collaborator

cmnbroad commented Feb 7, 2023

I don't have any issue with changing WeakReference to SoftReference, if it somehow actually fixes a problem. Is there any evidence that it does ? The gatk code isn't (and shouldn't be) sharing these across threads as far as I know, since that came up in the original code review.

We can't remove the backing reference though, since that was added to address a real performance issue (described in the comments). As I understand it, SoftReference is not required to have any different behavior than WeakReference. If thats correct, removing the backing reference would reintroduce the perf issue for at least some Java implementations. (BTW, I only discovered that issue through profiling - the only other manifestation is that everything just slows down a lot...)

I agree that the class here contract isn't clear; though, the synchronized does make it look like it's thread safe, but it isn't. I'm not sure why that was originally in there.

@vruano
Copy link
Contributor Author

vruano commented Feb 7, 2023

Thanks @cmnbroad. So far there is no evidence that Soft for Weak fixes the issue, may well in most cases but as you mentioned is not guarantee in all implementations.

I guess that there is two ways to proceed, either we actually make it thread safe adding back the last-requested hard reference or we simply scrap all the thread safety away and enforce the originally intended non-thread safe contract.

I think the most pragmatical would be the former; too make it truly thread-safe which is always an improvement on the existing code. But perhaps you see this as an opportunity to "make it right" although with the risk of breaking additional third party dependent code.

@lbergelson
Copy link
Member

@cmnbroad I think the Weak -> Soft reference switch makes sense, it definitely seems like the intent is that WeakReferences are collected eagerly and Soft are kept until there is memory pressure. It's possible that a jvm will implement it badly but I would guess that it's going to do basically what we want on any normal jvm we're likely to run on. I think it's very possible that the switch will fix the performance issue even if the explicit reference is removed.

With the Weak references the cache is effectively doing nothing, which explains why holding the last one specifically was necessary. If there is bad memory pressure we could still see thrashing here, but the alternative might be that we hold the reference and get OOM? Probably safest to keep the backing bases if you're worried about it, but I think in any case if we want the cache to work we should swap Weak -> Soft.

@vruano
Copy link
Contributor Author

vruano commented Feb 8, 2023

If we reinstate the 'backed' strong references would it make sense that these are updated and use regardless whether the invoking code calls get getReferenceSequence or getReferenceSequenceByRegion? The original code would only use and update the backed references in the latter is called. Is the a reason against doing so for both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants