Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Drain the transaction overlay#11654

Merged
dvdplm merged 1 commit into
masterfrom
dp/fix/memory-leak-when-warp-syncing
Apr 27, 2020
Merged

Drain the transaction overlay#11654
dvdplm merged 1 commit into
masterfrom
dp/fix/memory-leak-when-warp-syncing

Conversation

@dvdplm
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm commented Apr 26, 2020

When restoring a chunk during warp sync, drain the transaction overlay.
Fixes memory blowup seen after https://github.com/openethereum/openethereum/pull/11589 landed.

There's a discussion to be had about the semantics of the implementations of inject() and its docs. It can have significant side-effects beyond injecting independent extra database ops into a batch of writes.

@dvdplm dvdplm self-assigned this Apr 26, 2020
@dvdplm dvdplm requested a review from ordian April 26, 2020 18:07
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Apr 26, 2020
Copy link
Copy Markdown
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Nice catch! Sorry I totally missed that in https://github.com/openethereum/openethereum/pull/11589#discussion_r408067341. Submitting an empty transaction to drain the transaction_overlay does def sound hacky, create an issue for it?

@dvdplm
Copy link
Copy Markdown
Collaborator Author

dvdplm commented Apr 27, 2020

Submitting an empty transaction to drain the transaction_overlay does def sound hacky

It's pretty odd indeed. Is there no other way to do it?

@adria0
Copy link
Copy Markdown

adria0 commented Apr 27, 2020

When restoring a chunk during warp sync, drain the transaction overlay.
Fixes memory blowup seen after #11589 landed.

There's a discussion to be had about the semantics of the implementations of inject() and its docs. It can have significant side-effects beyond injecting independent extra database ops into a batch of writes.

so maybe inject deserves an unsafe tag?

@denisgranha
Copy link
Copy Markdown

based on the execution of this PR synchronizing mainnet, I can see that the memory leak is gone

@dvdplm dvdplm merged commit 114074c into master Apr 27, 2020
@dvdplm dvdplm deleted the dp/fix/memory-leak-when-warp-syncing branch April 27, 2020 11:51
dvdplm added a commit that referenced this pull request May 5, 2020
* master:
  Fix sccache server errors (#11675)
  Don't delete old db after migration (#11662)
  rename inject to drain_transaction_overlay (#11657)
  Drain the transaction overlay (#11654)
  vergen library seems to depend not only on the .git folder content but also on the git binary (#11651)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants