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

Improve external types and JSON codec #2401

Merged
merged 17 commits into from
Mar 30, 2023

Conversation

turbolent
Copy link
Member

Closes #2167

Description

  • Remove unnecessary link decoding from JSON-CDC codec. Only encoding is necessary
  • Add support for account links to cadence (add cadence.AccountLink and encoding in JSON-CDC)

Also:


  • 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

@github-actions
Copy link

github-actions bot commented Mar 24, 2023

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 2a3eac6
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Collapsed results for better readability

old.txtnew.txt
time/opdelta
CheckContractInterfaceFungibleTokenConformance-2113µs ± 0%111µs ± 0%~(p=1.000 n=1+1)
ContractInterfaceFungibleToken-238.8µs ± 0%38.5µs ± 0%~(p=1.000 n=1+1)
ExportType/composite_type-2323ns ± 0%472ns ± 0%~(p=1.000 n=1+1)
ExportType/simple_type-252.8ns ± 0%55.9ns ± 0%~(p=1.000 n=1+1)
InterpretRecursionFib-22.66ms ± 0%2.50ms ± 0%~(p=1.000 n=1+1)
NewInterpreter/new_interpreter-21.09µs ± 0%1.09µs ± 0%~(p=1.000 n=1+1)
NewInterpreter/new_sub-interpreter-2576ns ± 0%571ns ± 0%~(p=1.000 n=1+1)
ParseArray-27.74ms ± 0%8.24ms ± 0%~(p=1.000 n=1+1)
ParseDeploy/byte_array-212.5ms ± 0%11.2ms ± 0%~(p=1.000 n=1+1)
ParseDeploy/decode_hex-21.20ms ± 0%1.19ms ± 0%~(p=1.000 n=1+1)
ParseFungibleToken/With_memory_metering-2180µs ± 0%183µs ± 0%~(p=1.000 n=1+1)
ParseFungibleToken/Without_memory_metering-2141µs ± 0%142µs ± 0%~(p=1.000 n=1+1)
ParseInfix-27.01µs ± 0%7.00µs ± 0%~(p=1.000 n=1+1)
QualifiedIdentifierCreation/One_level-22.48ns ± 0%2.41ns ± 0%~(p=1.000 n=1+1)
QualifiedIdentifierCreation/Three_levels-2116ns ± 0%115ns ± 0%~(p=1.000 n=1+1)
RuntimeResourceDictionaryValues-25.14ms ± 0%5.12ms ± 0%~(p=1.000 n=1+1)
RuntimeScriptNoop-24.28µs ± 0%7.43µs ± 0%~(p=1.000 n=1+1)
SuperTypeInference/arrays-2297ns ± 0%296ns ± 0%~(p=1.000 n=1+1)
SuperTypeInference/composites-2132ns ± 0%133ns ± 0%~(p=1.000 n=1+1)
SuperTypeInference/integers-297.0ns ± 0%95.6ns ± 0%~(p=1.000 n=1+1)
ValueIsSubtypeOfSemaType-288.6ns ± 0%93.9ns ± 0%~(p=1.000 n=1+1)
 
alloc/opdelta
CheckContractInterfaceFungibleTokenConformance-248.9kB ± 0%48.9kB ± 0%~(all equal)
ContractInterfaceFungibleToken-223.2kB ± 0%23.2kB ± 0%~(all equal)
ExportType/composite_type-2136B ± 0%136B ± 0%~(all equal)
ExportType/simple_type-20.00B 0.00B ~(all equal)
InterpretRecursionFib-21.00MB ± 0%1.00MB ± 0%~(all equal)
NewInterpreter/new_interpreter-2768B ± 0%768B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-2200B ± 0%200B ± 0%~(all equal)
ParseArray-22.72MB ± 0%2.65MB ± 0%~(p=1.000 n=1+1)
ParseDeploy/byte_array-24.09MB ± 0%4.09MB ± 0%~(p=1.000 n=1+1)
ParseDeploy/decode_hex-2214kB ± 0%214kB ± 0%~(p=1.000 n=1+1)
ParseFungibleToken/With_memory_metering-228.9kB ± 0%28.9kB ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-228.9kB ± 0%28.9kB ± 0%~(all equal)
ParseInfix-21.92kB ± 0%1.92kB ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
RuntimeResourceDictionaryValues-22.28MB ± 0%2.28MB ± 0%~(p=1.000 n=1+1)
RuntimeScriptNoop-22.70kB ± 0%2.70kB ± 0%~(all equal)
SuperTypeInference/arrays-296.0B ± 0%96.0B ± 0%~(all equal)
SuperTypeInference/composites-20.00B 0.00B ~(all equal)
SuperTypeInference/integers-20.00B 0.00B ~(all equal)
ValueIsSubtypeOfSemaType-248.0B ± 0%48.0B ± 0%~(all equal)
 
allocs/opdelta
CheckContractInterfaceFungibleTokenConformance-2806 ± 0%806 ± 0%~(all equal)
ContractInterfaceFungibleToken-2370 ± 0%370 ± 0%~(all equal)
ExportType/composite_type-23.00 ± 0%3.00 ± 0%~(all equal)
ExportType/simple_type-20.00 0.00 ~(all equal)
InterpretRecursionFib-218.9k ± 0%18.9k ± 0%~(all equal)
NewInterpreter/new_interpreter-213.0 ± 0%13.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-24.00 ± 0%4.00 ± 0%~(all equal)
ParseArray-259.6k ± 0%59.6k ± 0%~(p=1.000 n=1+1)
ParseDeploy/byte_array-289.4k ± 0%89.4k ± 0%~(p=1.000 n=1+1)
ParseDeploy/decode_hex-263.0 ± 0%63.0 ± 0%~(all equal)
ParseFungibleToken/With_memory_metering-2768 ± 0%768 ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-2768 ± 0%768 ± 0%~(all equal)
ParseInfix-248.0 ± 0%48.0 ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
RuntimeResourceDictionaryValues-236.9k ± 0%36.9k ± 0%~(p=1.000 n=1+1)
RuntimeScriptNoop-243.0 ± 0%43.0 ± 0%~(all equal)
SuperTypeInference/arrays-23.00 ± 0%3.00 ± 0%~(all equal)
SuperTypeInference/composites-20.00 0.00 ~(all equal)
SuperTypeInference/integers-20.00 0.00 ~(all equal)
ValueIsSubtypeOfSemaType-21.00 ± 0%1.00 ± 0%~(all equal)
 

@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #2401 (3f90c88) into master (27e7705) will increase coverage by 0.05%.
The diff coverage is 91.52%.

❗ Current head 3f90c88 differs from pull request most recent head 09eea5d. Consider uploading reports for the commit 09eea5d to get more accurate results

@@            Coverage Diff             @@
##           master    #2401      +/-   ##
==========================================
+ Coverage   78.55%   78.60%   +0.05%     
==========================================
  Files         316      316              
  Lines       68596    68848     +252     
==========================================
+ Hits        53885    54118     +233     
- Misses      12911    12928      +17     
- Partials     1800     1802       +2     
Flag Coverage Δ
unittests 78.60% <91.52%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
runtime/common/memorykind_string.go 40.00% <ø> (ø)
runtime/common/metering.go 92.32% <ø> (ø)
runtime/sema/resources.go 88.44% <0.00%> (ø)
runtime/ast/expression.go 92.46% <50.00%> (ø)
runtime/convertTypes.go 87.45% <69.23%> (-2.18%) ⬇️
values.go 75.31% <71.42%> (-0.08%) ⬇️
runtime/convertValues.go 82.94% <87.50%> (+0.19%) ⬆️
runtime/sema/type.go 89.80% <91.66%> (-0.02%) ⬇️
encoding/json/decode.go 88.64% <100.00%> (+0.21%) ⬆️
encoding/json/encode.go 94.72% <100.00%> (+0.20%) ⬆️
... and 1 more

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@fxamacker fxamacker 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! I mostly focused on external types, values, and JSON-CDC encoding.

Only have one question and a few very minor nits.

types.go Outdated Show resolved Hide resolved
types.go Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
@fxamacker
Copy link
Member

@turbolent To confirm my understanding of this change's impact on CCF specs and CCF codec #2364:

  • CCF doesn't need to add support for AccountLink and PathLink values.
  • CCF needs to add encoding for type parameters in function type.

Is this correct?

types.go Show resolved Hide resolved
turbolent and others added 3 commits March 30, 2023 15:06
Co-authored-by: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
@turbolent
Copy link
Member Author

@fxamacker

To confirm my understanding of this change's impact on CCF specs and CCF codec #2364:

* CCF doesn't need to add support for `AccountLink` and `PathLink` values.

* CCF needs to add encoding for type parameters in function type.

Is this correct?

  • Yes, CCF does not need support for links for now. Encoding of links to JSON is only used by the Playground for now, but will eventually be refactored, so I don' think we'll need support for them in CCF. We can add it later if needed.

  • Yes, we should maybe encode type parameters for function types in CCF from the get-go

@turbolent turbolent merged commit 5bfce97 into master Mar 30, 2023
@turbolent turbolent deleted the bastian/improve-link-export-import branch September 1, 2023 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add static type for Cadence external PathLink value
3 participants