feat(ethexe/processor): instrument code lazily inside processing#5396
feat(ethexe/processor): instrument code lazily inside processing#5396
Conversation
Changed Files
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a lazy instrumentation mechanism for program code within the processor. By deferring the instrumentation process until the code is actually required for execution, the system improves efficiency and robustness. The changes involve updating the run context to handle instrumentation dynamically and ensuring that missing instrumented code is generated on-the-fly from the original source. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements on-the-fly instrumentation for Gear programs within the ethexe processor. When instrumented code or metadata is missing from the database, the processor now fetches the original code, performs instrumentation using a Wasmtime instance, and persists the results back to the database. Feedback focuses on performance optimizations: specifically, noting that saving results to an overlaid database (common in RPC calls) may lead to redundant heavy instrumentation, and suggesting the reuse of Wasmtime instances across a processing pass to reduce instantiation overhead.
| db.set_instrumented_code( | ||
| ethexe_runtime_common::VERSION, | ||
| code_id, | ||
| instrumented_code.clone(), | ||
| ); | ||
| db.set_code_metadata(code_id, code_metadata.clone()); |
There was a problem hiding this comment.
When db is an overlaid database (e.g., during RPC execute_for_reply), these set calls only update the in-memory overlay. This means every RPC call for the same uninstrumented program will re-perform instrumentation. Since instrumentation is a computationally heavy operation, this might be a performance bottleneck for RPC. Ideally, deterministic instrumentation results should be cached in the base database, but the current Database abstraction makes this difficult without bypassing the overlay. Given this is a fallback path, it's acceptable, but worth noting for future optimization.
References
- Code instrumentation is recognized as a computationally heavy 'main operation' in this repository, and its performance impact should be considered during implementation.
| .original_code(code_id) | ||
| .ok_or(ProcessorError::MissingOriginalCodeForProgram(code_id))?; | ||
|
|
||
| let mut instance = instance_creator.instantiate()?; |
There was a problem hiding this comment.
Instantiating a new Wasmtime instance for every uninstrumented code blob can be expensive. Given that code instrumentation is a heavy operation, this adds significant overhead. In spawn_chunk_execution, if a chunk contains multiple programs with uninstrumented code, this will happen multiple times. Although InstanceCreator::instantiate is optimized via InstancePre, you might consider instantiating the runtime once per chunk processing pass and reusing it for all required instrumentations within that pass to reduce overhead.
References
- Code instrumentation is recognized as a computationally heavy 'main operation' in this repository, and its performance impact should be considered during implementation.
|
@cursor review |
|
@codex review |
|
@claude review |
|
Claude finished @playX18's task in 1m 49s —— View job Code Review
Summary: This PR adds a lazy fallback in medium: misleading error when File: let (instrumented_code, code_metadata) = instance
.instrument(&original_code)?
.ok_or(ProcessorError::MissingInstrumentedCodeForProgram(code_id))?;
That said, this path is guarded upstream: low: RPC re-instrumentation on every call File: In This is acceptable for an uncommon fallback path (missing instrumented code after a runtime upgrade should be temporary), but worth a comment so future readers don't wonder why writes appear to be no-ops for RPC callers. low: partial-state non-issue but worth noting If low: test only covers the happy path File: The new test verifies that after
nit:
Overall this is a clean fix. The logic is sound given that |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5207bb5. Configure here.
|
Claude finished @playX18's task in 2m 34s —— View job Code Review
Summary: This PR extracts medium: violates the documented "no KV writes from processor" invariant File: The crate-level doc currently reads: Both medium: File: let (instrumented_code, code_metadata) = instance
.instrument(&original_code)?
.ok_or(ProcessorError::MissingInstrumentedCodeForProgram(code_id))?;
In practice this path is guarded upstream — low: overlaid DB writes are ephemeral — re-instruments on every RPC call File:
low: parallel chunk execution may redundantly instrument the same File: Multiple programs in a chunk may share the same low: new test only covers the happy path File: The test verifies that after
Overall this is a clean, well-targeted fix. The main asks are: update the broken invariant comment in the crate doc, and rename the error produced when |
Resolves #4681
Note
Medium Risk
Adds on-the-fly WASM instrumentation and DB writes inside the hot queue-processing path, which could affect determinism/performance and introduces new failure mode when original code is absent.
Overview
Enables lazy code instrumentation during program execution: when processing a program queue (including overlay mode), if the current-runtime
instrumented_code/code_metadatais missing in the DB, the processor now instruments the storedoriginal_codevia a runtime instance and persists the results.This threads
InstanceCreatorintoinstrumented_code_and_metadata, exposes it onCommonRunContext, adds a newMissingOriginalCodeForProgramerror, and includes a regression test ensuring processing populates missing instrumentation for valid code.Reviewed by Cursor Bugbot for commit 5207bb5. Bugbot is set up for automated code reviews on this repo. Configure here.