fix: Add perf tests for escrow create and escrow finish - #7789
fix: Add perf tests for escrow create and escrow finish#7789TimothyBanks wants to merge 17 commits into
Conversation
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Had a look at this
Two correctness issues flagged inline: nanosecond timing is silently truncated to milliseconds by Event::notify (both timing sites in WasmVM.cpp), and the destructor unconditionally prints to stdout for every test instance, not just the perf tests.
Review by ReviewBot 🤖
Review by Claude Sonnet 4.6 · Prompt: V15
| auto const ret = | ||
| vm.check(wasmCode, hfs, funcName, params, createWasmImport(hfs), hfs.getJournal()); | ||
|
|
||
| hfs.executionTimeEvent("preflightEscrowWasm") |
There was a problem hiding this comment.
This was a surprise to me too, but the caller in EscrowCreate.cpp is a mock. So it doesn't have the real impl of executionTimeEvent.
HostFunctions mock(ctx.j);
auto const re = preflightEscrowWasm(code, mock, escrowFunctionName);
There was a problem hiding this comment.
That does seem a bit intentional?
|
This is from claude. I think it makes sense. Please take a look. The execution-timing metric can't resolve these calls — it reports 0.
I measured it locally (Release build, wasmi, That's ~11x and ~28x below the 1ms resolution, and the test output confirms it — both perf cases print |
| { | ||
| if (count > 0) | ||
| { | ||
| std::cout << "Mean (ms): " << meanMs() << "\n"; |
There was a problem hiding this comment.
Nit: Claude complained see this printed too many times for unrelated tests.
There was a problem hiding this comment.
Addressed.
As for the resolution of the metrics, the best I can do is fake the values as milliseconds. That is pass in microseconds. StatsD is set at millisecond resolution.
There was a problem hiding this comment.
Claude used kAllHostFunctionsWasmHex as the example. As the name says, this wasm code tests all the host functions. I think a fair amount of "normal" wasm code might be less expensive. Assuming claude's measured numbers are real, we will have a lot of wasm executions in the tens of microseconds range. If StatsD's time events in at millisecond resolution, and reporting is not free (google says: "Sending a StatsD metric takes roughly 1 to 5 microseconds of application CPU time.), then we need a different approach. Does it have to be a StatsD time event, or sent every time... I don't know the receiving side to give a good suggestion. On the wasm VM side though, we just want to have some basic statistics, and also get some information if some wasm code takes relatively longer time, like 10 ms.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
2 participants
High Level Overview of Change
Adds very simple perf test around escrow create and escrow finish calls.
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)