Skip to content

#194: Resolved Lookup library bug for JDK 21+#199

Open
zack-rma wants to merge 8 commits intomainfrom
bugfix/memory_issue
Open

#194: Resolved Lookup library bug for JDK 21+#199
zack-rma wants to merge 8 commits intomainfrom
bugfix/memory_issue

Conversation

@zack-rma
Copy link
Contributor

@zack-rma zack-rma commented Jan 28, 2026

Description

Adds compatibility library to resolve memory and CPU usage bug introduced by transitive Lookup library dependency affecting JDK 21+. Swaps rma.util.Lookup usage for Netbeans Lookup API using lookup-compat library.

Motivation and Context

Contributes to resolve #194

Types of changes

Yes/No Pull Request Type Description
Yes Bug fix non-breaking change which fixes an issue
New feature non-breaking change which adds functionality
Breaking change fix or feature that would cause existing functionality to change
Documentation change non-breaking change which modifies or updates documentation
New tests new unit tests, test scenarios, or test case documentation
Triggers regression testing change affects downstream modules and will require regression testing
Other

How Has This Been Tested?

Verified using WRIMS comparison test. CPU and memory usage compared before and after change using VisualVM. Went from ~100% CPU time for original lookup library to ~0% with Netbeans library.

Notes for Reviewers

Please consider the following when reviewing this PR:

  • Correctness: Does the code do what it claims? Are edge cases handled appropriately?
  • Clarity: Is the code readable, maintainable and aligned with SOLID design principles?
  • Impact: Will this change affect other parts of the system? Any potential regressions?
  • Testing: Are the test cases sufficient and appropriate? Are there gaps in coverage?
  • Documentation: Does this require updates to code comments, README, or other docs?

@dwr-zroy
Copy link
Collaborator

Not sure this fully resolves #194. I still see memory expanding while the model is solving. This should be after all DSS data has been loaded into memory.

See below for the upward trend from VisualVM.

image

I also see the memory footprint of rma.util.lookup.Lookup$Template and rma.util.lookup.AbstractLookup$ReferenceToResult growing while the run progresses. We also do not get very far into the run before we start to have responsiveness issues.

@dwr-zroy
Copy link
Collaborator

Looking at a heapdump, it looks like we still have a lot of rma.utils.lookup.AbstractLookup$ReferenceToResult sticking around as weakly-referenced.

I'll say: I am not a Java expert, and I am sure I am missing the details here. I am way outside of my own expertise.

image image

@jmdegeorge
Copy link
Contributor

Not sure this fully resolves #194. I still see memory expanding while the model is solving. This should be after all DSS data has been loaded into memory.

See below for the upward trend from VisualVM.

image I also see the memory footprint of `rma.util.lookup.Lookup$Template` and `rma.util.lookup.AbstractLookup$ReferenceToResult` growing while the run progresses. We also do not get very far into the run before we start to have responsiveness issues.

Which project and configuration did you run where you hit these results? We probably need to include it in our testing suite so we can replicate the same results you are hitting on our end.

@dwr-zroy
Copy link
Collaborator

Which project and configuration did you run where you hit these results? We probably need to include it in our testing suite so we can replicate the same results you are hitting on our end.

Using the compute_calsim3-dcr-9.6.0-10yr.feature file, and then just using VisualVM to monitor things. Not sure if you need more detail to replicate.

Copy link

@rma-psmorris rma-psmorris left a comment

Choose a reason for hiding this comment

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

Comments

@rma-psmorris
Copy link

Not sure this fully resolves #194. I still see memory expanding while the model is solving. This should be after all DSS data has been loaded into memory.

@dwr-zroy - this PR resolves the known CPU issue with lookup, further investigation is needed into the memory issue.

@dwr-zroy
Copy link
Collaborator

Not sure this fully resolves #194. I still see memory expanding while the model is solving. This should be after all DSS data has been loaded into memory.

@dwr-zroy - this PR resolves the known CPU issue with lookup, further investigation is needed into the memory issue.

Will there be a separate PR, the description of this PR says it addresses memory as well.

@rma-psmorris
Copy link

Not sure this fully resolves #194. I still see memory expanding while the model is solving. This should be after all DSS data has been loaded into memory.

@dwr-zroy - this PR resolves the known CPU issue with lookup, further investigation is needed into the memory issue.

Will there be a separate PR, the description of this PR says it addresses memory as well.

Yes, we will want this PR merged to resolve lookup and the follow-up work on memory should be a new PR.

@zack-rma
Copy link
Contributor Author

Screenshot 2026-01-28 163212 Screenshot 2026-01-28 163256 Screenshot 2026-01-28 163502

@dwr-zroy I've run a few tests on the compute you referenced, but have not seen the same results.

I do see a few WeakReference maps, but they are negligible in size. I'm also not seeing the increasing memory usage you were experiencing. Can you try a clean and fresh run of the compute to see if you're still getting these issues on the latest changes in this branch? It's possible the library replacement wasn't taking effect, as I haven't seen any of the old lookup library present within the runs I've conducted.

A quick tell on whether the replacement worked successfully is whether the Active Reference Queue Daemon thread has any associated CPU time (>0ms is a failed replacement).

@dwr-zroy
Copy link
Collaborator

dwr-zroy commented Jan 29, 2026

@zack-rma I pulled latest, cleaned and re-tested, and I am seeing the same as you. Thanks.

image image

@zack-rma zack-rma requested a review from rma-psmorris January 29, 2026 17:47
@zack-rma zack-rma moved this to Needs Review in WRIMS 3 Development Jan 29, 2026
Copy link

@rma-psmorris rma-psmorris left a comment

Choose a reason for hiding this comment

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

Comment about grep usage

run: |
./gradlew -PcvmUserId="dwr-wrims-build" -PcvmPassword="${{ secrets.WRIMS_ENGINE_DEPENDENCIES_TOKEN }}" \
:wrims-core:dependencyInsight --dependency lookup --configuration runtimeClasspath \
| grep -F "^mil\.army\.usace\.hec:lookup:[^:\s]+$" && exit 1 || exit 0

Choose a reason for hiding this comment

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

don't you need grep -E to use regex? Throw in a q for quiet as you dont need stdout.
https://www.gnu.org/software/grep/manual/grep.html#index-_002dE

Choose a reason for hiding this comment

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

-F will look for literal
-F
--fixed-strings
Interpret patterns as fixed strings, not regular expressions. (-F is specified by POSIX.)
https://www.gnu.org/software/grep/manual/grep.html#index-_002dF

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

@zack-rma zack-rma requested a review from rma-psmorris February 2, 2026 18:52
@dwr-zroy dwr-zroy added the bug Something isn't working label Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

Memory use

4 participants