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

RMS-8495: Komprise Memory leak #618

Merged
merged 5 commits into from
Sep 15, 2023
Merged

Conversation

scribe
Copy link
Contributor

@scribe scribe commented Sep 11, 2023

Some work with a profiler detected a handful of issues:

  • A whole bunch of inner cases were marked static so the outer class can be GC'd
  • Replace Arrays.asList(new String[] {} with Collections.emptyList() to avoid unnecessary allocations
  • Replace anonymous new Function<…> with static classes to allow the GC to run correctly
  • Replace single item Arrays.asList with List.of which should bound allocation
  • Remove tons of unused import statements which allowed removal of a sub-project dependency that wasn't actually used.
  • Replace Charset.forName("UTF-8") which requires a parser with StandardCharsets.UTF_8 which does not.

All of these together have left the profiler graphs in significantly better shape.

Edit: I ran tests on my laptop but I really want to see the tests run in Jenkins to give me better feelings.

@scribe
Copy link
Contributor Author

scribe commented Sep 11, 2023

Looks like I have a merge issue merging with master. Time to clean it up.

@david-limbach
Copy link

@scribe Seems like 99% of the changes are converting:
private class
to
private static class

Is this really necessary?

@scribe
Copy link
Contributor Author

scribe commented Sep 11, 2023

@scribe Seems like 99% of the changes are converting: private class to private static class

Is this really necessary?

If an inner class is not static it needs to be GC'd before the outer class can be marked for GC. What I'm worried about is something we return the lives a long time that keeps the outer class from going away.

@scribe scribe merged commit bf93957 into SpectraLogic:master Sep 15, 2023
4 checks passed
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