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

Remove log statements that are keeping references to objects for too long #4705

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Nov 18, 2022

Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

Improvement in terms of reducing java heap used,
since the logs were keeping reference to blocks sent by newPayload, that causes high memory consumption during initial sync, and could be one of the causes that prevent to complete snap sync on low spec machines. Exceptions are also logged by the backward sync, so there is no loss of information.

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

…much time

Improvement in terms of reducing java heap used,
since the logs were keeping reference to blocks sent by newPayload,
that causes high memory consumption during initial sync,
and could be one of the causes that prevent to complete snap sync on low spec machines.
Exceptions are also logged by the backward sync, so there is no loss of information.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 marked this pull request as ready for review November 18, 2022 16:12
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

do your bwsync PRs add some logging in backward sync that will inform us of failures since we are dropping these?

@fab-10
Copy link
Contributor Author

fab-10 commented Nov 18, 2022

Yes errors are already logged in backward sync

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

🚢 I have been burning in the prior version of this PR overnight and noticed significantly less memory pressure on low spec machine

@fab-10 fab-10 enabled auto-merge (squash) November 18, 2022 18:31
@fab-10 fab-10 merged commit 098558a into hyperledger:main Nov 18, 2022
@fab-10 fab-10 deleted the memory-fix branch November 18, 2022 18:57
macfarla pushed a commit to macfarla/besu that referenced this pull request Jan 10, 2023
…long (hyperledger#4705)

* Remove log statements that are keeping references to objects for too much time

Improvement in terms of reducing java heap used,
since the logs were keeping reference to blocks sent by newPayload,
that causes high memory consumption during initial sync,
and could be one of the causes that prevent to complete snap sync on low spec machines.
Exceptions are also logged by the backward sync, so there is no loss of information.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
…long (hyperledger#4705)

* Remove log statements that are keeping references to objects for too much time

Improvement in terms of reducing java heap used,
since the logs were keeping reference to blocks sent by newPayload,
that causes high memory consumption during initial sync,
and could be one of the causes that prevent to complete snap sync on low spec machines.
Exceptions are also logged by the backward sync, so there is no loss of information.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants