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

[config change] use MessageCommitMode when executing future head block messages #2705

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

magicxyyz
Copy link
Contributor

@magicxyyz magicxyyz commented Sep 26, 2024

Fixes NIT-2812
Pulls: OffchainLabs/go-ethereum#362
Includes: #2712

This PR:

  • Fixes use of MessageRunMode values, so as MessageCommitMode is used when, and only when, the message is part of a soon-to-be head block. Previously, newly sequenced / synced messages were executed in MessageReplayMode - newly activated / set-cached stylus programs were not cached in long term cache (only in LRU).

  • Improves repopulating of long term cache after node restart - if program is onchain marked as cached, if its wasm is found in LRU then it is also added to long term cache. That can happen e.g. when a ephemeral call to cached program precedes its onchain execution.

  • Adds tests for stylus long term cache + for repopulating long term cache from LRU cache.

  • Adds metrics for Stylus long term cache (merged from Diego's draft: Stylus cache improvements #2712)

  • Adds config to disable collection of Stylus metrics from Go side (also from the Diego's draft)

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Sep 26, 2024
Copy link
Contributor

@diegoximenes diegoximenes left a comment

Choose a reason for hiding this comment

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

LGTM overall, requesting changes because I miss a test for this fix

system_tests/state_fuzz_test.go Show resolved Hide resolved
execution/gethexec/executionengine.go Show resolved Hide resolved
@diegoximenes
Copy link
Contributor

I created a draft PR that exposes Stylus long term cache metrics that can be helpful when implementing tests in this PR.

I didn't implement tests in my PR since it requires that long term caching is working properly, which is not true in the master branch 😬

In case you want to use what I developed you can get the changes from my branch into your branch, and then continue and implement the tests.
You can merge my PR into your branch.


// See if the item is in the long term cache
if let Some(item) = cache.long_term.get(&key) {
return Some(item.data());
}

// See if the item is in the LRU cache, promoting if so
if let Some(item) = cache.lru.get(&key) {
let data = item.data();
if let Some(item) = cache.lru.peek(&key).cloned() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This codepath clones data twice: once here in the "get" and the other when returning item.data().
Cloning the entry_size_estimate_bytes is o.k., but we don't want to clone module and engine unnecessarily.
This is where rust gets you :)
There are probably some solutions that would avoid cloning result of the peek, but I think simplest would probably be if you can avoid cloning in item.data() because item itself is discarded right after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tsahi pointed out that there's one more unnecessary clone, so that's not fixed yet, working on it :)

@magicxyyz magicxyyz changed the title use MessageCommitMode when executing future head block messages [config change] use MessageCommitMode when executing future head block messages Oct 3, 2024
@magicxyyz magicxyyz marked this pull request as draft October 4, 2024 10:45
@magicxyyz magicxyyz marked this pull request as ready for review October 4, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants