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

#648: switch to the regular toString for logging to prevent excessive memory usage #649

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

otarsko
Copy link
Member

@otarsko otarsko commented Oct 12, 2022

Replace serialization of config from Yaml to regular toString.

Comparison of memory usage:

Before (OutOfMemory ons serialization)

Size 685,768,736 B

After (no OutOfMemory)

Size 1,572,864,032 B

Example of serialized config in the history

image

@otarsko otarsko requested review from kwin and ghenzler October 12, 2022 09:51
@otarsko otarsko self-assigned this Oct 12, 2022
@sonarcloud
Copy link

sonarcloud bot commented Oct 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.0% 90.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@kwin kwin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Is the dump from above with or without #647?

@otarsko
Copy link
Member Author

otarsko commented Oct 12, 2022

@kwin that's without - testing of #647 is pending (and will be done on top of these changes)

@otarsko otarsko removed the request for review from ghenzler October 13, 2022 07:52
@@ -83,7 +83,7 @@ public void persistHistory(PersistableInstallationLogger installLog) {

String mergedAndProcessedConfig = installLog.getMergedAndProcessedConfig();
if (StringUtils.isNotBlank(mergedAndProcessedConfig)) {
JcrUtils.putFile(historyNode, "mergedConfig.yaml", "text/yaml",
JcrUtils.putFile(historyNode, "mergedConfig.txt", "text/plain",
Copy link
Member

Choose a reason for hiding this comment

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

so this is a shame to have only a "toString" representation saved - isn't it possible to somehow generate the yaml without too much memory impact? (IIRC today snippets of this file can be used to test things and do some "round-tripping")

Copy link
Member

Choose a reason for hiding this comment

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

suggestions welcome, SnakeYAML itself has huge overhead!

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we could fairly easily write a toString method, that creates valid yaml (instead of just using ToStringBuilder) - but we can also do that separately, not the most important to have immediately

Copy link
Member Author

Choose a reason for hiding this comment

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

@ghenzler I've created #651 to improve toString. Hope it's fine.

@kwin kwin merged commit 95b755c into develop Nov 4, 2022
@kwin kwin deleted the bugfix/648-optimize-memory-usage branch November 4, 2022 10:27
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