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

[vruano]

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
• [X] 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.

[lbergelson]

@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.

[cmnbroad]

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]

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]

@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]

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?