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

Computation reports for Cadence scripts & transactions #609

Merged
merged 13 commits into from
Mar 21, 2024

Conversation

m-Peter
Copy link
Contributor

@m-Peter m-Peter commented Mar 12, 2024

Completes: 1st milestone of onflow/developer-grants#178
Closes: #526

Description

Introduces a new endpoint under http://localhost:8080/emulator/computationReport, to display computation reports. For example:

computation-profile


For contributor use:

  • Targeted PR against master branch
  • Linked to GitHub issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the GitHub PR explorer
  • Added appropriate labels

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 54.27136% with 91 lines in your changes are missing coverage. Please review.

Project coverage is 50.93%. Comparing base (d9395dd) to head (c21ffd9).

Files Patch % Lines
emulator/computation_report.go 49.29% 72 Missing ⚠️
server/utils/emulator.go 0.00% 10 Missing ⚠️
server/server.go 0.00% 5 Missing and 1 partial ⚠️
emulator/blockchain.go 92.68% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #609      +/-   ##
==========================================
+ Coverage   50.80%   50.93%   +0.13%     
==========================================
  Files          32       33       +1     
  Lines        4224     4421     +197     
==========================================
+ Hits         2146     2252     +106     
- Misses       1907     1996      +89     
- Partials      171      173       +2     
Flag Coverage Δ
unittests 50.93% <54.27%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Looks good. Some comments:

  • It might be good to link to this function as an example on how to get the computation from the intensities https://github.com/onflow/flow-go/blob/master/fvm/meter/computation_meter.go#L32
  • The naming: "profiling" hints to the profiling where you can see results per line of code executed which is a bit misleading. However if I understand correctly you intend to work towards that. If that is the case I'm ok with the name.


// transformIntensities maps a numeric common.ComputationKind value
// to a human-friendly string value.
func transformIntensities(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish flow-go already offered ComputationKind names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I think this is not possible. The main problem is that ComputationKind is defined in Cadence repository and it uses the string cmd tool to generate the String representation for the values defined there. See: https://github.com/onflow/cadence/blob/master/runtime/common/compositekind.go#L27
Only the ComputationKind values defined in Cadence offer a human-readable name: https://github.com/onflow/cadence/blob/master/runtime/common/computationkind_string.go.

Comment on lines 86 to 94
expectedIntensities := map[string]uint{
"FunctionInvocation": 2,
"GetAccountContractCode": 1,
"GetCode": 1,
"GetOrLoadProgram": 3,
"GetValue": 221,
"ResolveLocation": 1,
"Statement": 1,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not compare all the values, as the test might become fragile.
I would maybe omit GetValue and perhaps GetOrLoadProgram as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍
Updated in 5dc4699

Comment on lines 144 to 160
"AllocateStorageIndex": 2,
"CreateCompositeValue": 2,
"CreateDictionaryValue": 1,
"EmitEvent": 73,
"EncodeEvent": 1,
"EncodeValue": 2336,
"FunctionInvocation": 9,
"GenerateAccountLocalID": 1,
"GenerateUUID": 1,
"GetAccountContractCode": 1,
"GetCode": 1,
"GetOrLoadProgram": 3,
"GetValue": 2249,
"ResolveLocation": 1,
"SetValue": 2473,
"Statement": 11,
"TransferCompositeValue": 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not check:

  • EncodeValue
  • GetValue
  • SetValue
  • AllocateStorageIndex
  • GetOrLoadProgram

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍
Updated in 5dc4699

@m-Peter
Copy link
Contributor Author

m-Peter commented Mar 13, 2024

@janezpodhostnik I have updated the naming pattern to computation report in 7a259eb, for types / variables / flags etc. I have also linked the relevant flow-go function for getting the computation from intensities.

@m-Peter m-Peter changed the title Computation profiling report for Cadence scripts & transactions Computation reports for Cadence scripts & transactions Mar 15, 2024
@m-Peter m-Peter force-pushed the cadence-computation-profiling-report branch from 61361f0 to a3c399f Compare March 15, 2024 15:42
Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Very nice work, this is an awesome feature! 🎉

@m-Peter m-Peter force-pushed the cadence-computation-profiling-report branch 3 times, most recently from c21ffd9 to f7eb861 Compare March 20, 2024 18:20
@m-Peter m-Peter force-pushed the cadence-computation-profiling-report branch from f7eb861 to 963b2be Compare March 21, 2024 14:17
@sideninja sideninja merged commit 70db91e into onflow:master Mar 21, 2024
3 checks passed
@turbolent turbolent added the Feature A new user feature or a new package API label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A new user feature or a new package API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return computation used for scripts/transactinon on emulator
5 participants