Execution Cost Flamegraph #1834
Conversation
|
Docker tags |
Benchmark for fc0e3b0Click to view benchmark
|
There was a problem hiding this comment.
Really nice output! :)
But before considering merging this, I think we need to be more careful about:
- What we're adding to our interfaces and the consequences. Indeed, I don't think we want to add lots of internal engine models to the transaction receipt, as it makes it much harder to change them without breaking things.
- Reducing performance impact for normal preview (which full nodes do a lot of)
Instead:
- Perhaps we may wish to flatten lots of things out into Strings on the way to the receipt
- OR consider pushing the flamegraph generation further into the engine, to improve perf and reduce the overhead.
If we want a more structured execution trace for other reasons, (which might be really cool, and was my original model for preview), we should start by considering what the format of its DTO should be - rather than just exposing engine models on the receipt interface.
radix-engine/src/system/system_modules/costing/costing_module.rs
Outdated
Show resolved
Hide resolved
|
|
||
| // Add an entry for the more detailed execution cost | ||
| cost_breakdown.detailed_execution_cost_breakdown.push(( | ||
| self.current_depth, |
There was a problem hiding this comment.
I'd rather have this method take a &KernelApi or a current_depth parameter, than risk the depth getting out of sync.
(That would enable us to delete current_depth from the costing module state, and clear up a lot of possibly flakey code).
There was a problem hiding this comment.
Sounds good, will do that.
There was a problem hiding this comment.
I would recommend leaving this change for a later PR since it's quite a big change and the logic around the depth I think is somewhat simple and can easily be verified.
There was a problem hiding this comment.
Looks like you did fix this in the end, thanks :).
But yeah, I think it makes sense to change in this PR so we don't introduce something and then remove it which creates unneeded churn.
radix-engine/src/system/system_modules/costing/costing_module.rs
Outdated
Show resolved
Hide resolved
|
@dhedey Thank you for the review! I'm responding to your review comment in this comment.
So, I thought about this a little bit more. I think that I would like not to include this information in the transaction receipt. Instead, I want to propose that execution returns two transactions receipts instead of one: The This would address the backward compatibility issues we talked about and also any concerns we have related to the size of the preview receipts since adding this detailed execution cost information to the transaction receipt will make it massive. The idea would be as follows: information that we're ready to persist, write DTOs for, and expose to clients would be part of the What do you think?
Totally agree, need to rework this bit. I'm now concerned about how much the size of the receipt increases with this information and about the performance impact. So, will rework this bit.
At the current moment of time, I would recommend that we do not flatten them into strings and keep them this way so that we can extract as much information out of them as needed during the analysis. You'll notice that only a small amount of the information is used in the flamegraph and a large part goes unused, this is intentionally done so that we have this information when we need further analysis. If we do the
To understand, in this case the flamegraph would not be generated from the receipt, it would be generated on the fly (perhaps in the costing module) as the transaction is executed?
Yup, I agree, I'm personally very much in favor of us not exposing this information at the time being since it's still not very mature and can very much change and instead we would surface it to tests in a This information is not currently in a state where I'd say "yeah let's commit to this format long term" and it could very much change in the future because we want to surface more information. So, this is where the debug transaction receipt idea comes from. |
| @@ -871,6 +867,7 @@ impl<C: SystemCallbackObject> KernelCallbackObject for System<C> { | |||
| let limits_module = { LimitsModule::from_params(system_parameters.limit_parameters) }; | |||
|
|
|||
| let costing_module = CostingModule { | |||
| current_depth: 0, // TODO: Is it correct to assume that the current depth is zero here? | |||
There was a problem hiding this comment.
I think this was missed? We should resolve this TODO - either commenting why we can assume this, or handling it properly?
Seems reasonable to me that init sets current_depth to 0 though 👍
There was a problem hiding this comment.
Yup I missed this, I've removed the TODO comment and will add a comment explaining why we set it to zero.
| @@ -1125,6 +1127,15 @@ impl<C: SystemCallbackObject> KernelCallbackObject for System<C> { | |||
| None | |||
| }; | |||
|
|
|||
| let debug_information = match (&costing_module.detailed_cost_breakdown,) { | |||
| (Some(detailed_cost_breakdown),) => Some(TransactionDebugInformation { | |||
| detailed_execution_cost_breakdown: detailed_cost_breakdown | |||
There was a problem hiding this comment.
If we create a costing_module.deconstruct() which splits out the detailed_cost_breakdown from the fee_reserve etc then can we avoid the expensive clone here?
There was a problem hiding this comment.
Sounds good to me. In this case avoiding the clone didn't require adding a new method. The current context already owns the CostingModule so I can simply operate over the owned value instead of operating over a reference and this avoids the clone.
|
|
||
| // Add an entry for the more detailed execution cost | ||
| cost_breakdown.detailed_execution_cost_breakdown.push(( | ||
| self.current_depth, |
There was a problem hiding this comment.
Looks like you did fix this in the end, thanks :).
But yeah, I think it makes sense to change in this PR so we don't introduce something and then remove it which creates unneeded churn.
Summary
This PR adds a more detailed execution cost breakdown and also adds the ability for a flamegraph to be generated it.
(Save the above SVG locally to view it correctly)
Details
This PR adds the ability for the costing module to collect more detailed information on the cost unit consumption during execution. This information is then surfaced in transaction receipts and can be graphed and represented as a flamegraph/flamechart. Note that in flamegraphs the events are ordered alphabetically and not in the order in which they occured.
Please note that this PR makes a breaking change to transaction receipts. I suggest that we keep this breaking change until we introduce the transaction receipt DTO.