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

Interpreting for basic attachment operations #2113

Merged

Conversation

dsainati1
Copy link
Contributor

Part of #357 and #2062

This implements runtime semantics for attaching, accessing and removing attachments


  • 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

Copy link
Contributor Author

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

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

This is very much still a draft; need a lot more tests before I am comfortable with this, but I wanted to put it up for preliminary feedback on the methodology.

runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
Comment on lines 15221 to 15222
interpreter.SharedState.inAttachmentIteration[v] = struct{}{}
defer delete(interpreter.SharedState.inAttachmentIteration, v)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every time we enter an iteration over a composite's attachment we store that composite in this set, and remove it when we're done. This can't just be a bool like the storage iteration because we only want to restrict mutations of this specific composite's attachments, not all attachments generally.

@dsainati1 dsainati1 marked this pull request as ready for review November 14, 2022 16:05
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work!

runtime/interpreter/interpreter_expression.go Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Show resolved Hide resolved
runtime/interpreter/value.go Show resolved Hide resolved
runtime/sema/checker.go Outdated Show resolved Hide resolved
Base automatically changed from sainati/attachments-check-add-remove to feature/attachments December 12, 2022 15:21
@turbolent
Copy link
Member

CI is broken because of the automatic base branch change (GitHub bug), merging master should fix it and retrigger CI

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #2113 (7816074) into feature/attachments (3abda1a) will increase coverage by 0.12%.
The diff coverage is 73.04%.

@@                   Coverage Diff                   @@
##           feature/attachments    #2113      +/-   ##
=======================================================
+ Coverage                77.75%   77.87%   +0.12%     
=======================================================
  Files                      312      313       +1     
  Lines                    66251    66529     +278     
=======================================================
+ Hits                     51513    51811     +298     
+ Misses                   12948    12924      -24     
- Partials                  1790     1794       +4     
Flag Coverage Δ
unittests 77.87% <73.04%> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
runtime/interpreter/errors.go 55.21% <0.00%> (-2.45%) ⬇️
runtime/interpreter/value_accountreference.go 35.07% <0.00%> (ø)
runtime/sema/checker.go 92.39% <ø> (+0.10%) ⬆️
runtime/interpreter/value.go 69.27% <65.98%> (+0.40%) ⬆️
runtime/interpreter/function.go 72.28% <66.66%> (+0.33%) ⬆️
runtime/interpreter/interpreter_expression.go 84.52% <66.98%> (-1.75%) ⬇️
runtime/sema/accesscheckmode.go 85.71% <85.71%> (ø)
runtime/sema/elaboration.go 75.25% <85.71%> (+0.47%) ⬆️
runtime/interpreter/interpreter_statement.go 91.68% <87.50%> (+0.36%) ⬆️
runtime/interpreter/interpreter.go 89.33% <92.00%> (+0.13%) ⬆️
... and 19 more

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

runtime/interpreter/sharedstate.go Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Show resolved Hide resolved
runtime/interpreter/value.go Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Dec 12, 2022

Cadence Benchstat comparison

This branch with compared with the base branch onflow:feature/attachments commit 3abda1a
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-2114µs ± 1%114µs ± 2%~(p=0.259 n=7+7)
ContractInterfaceFungibleToken-239.0µs ± 0%39.3µs ± 3%~(p=0.534 n=6+7)
InterpretRecursionFib-22.50ms ± 1%2.49ms ± 1%~(p=0.836 n=6+7)
NewInterpreter/new_interpreter-21.10µs ± 0%1.10µs ± 1%−0.73%(p=0.037 n=6+7)
NewInterpreter/new_sub-interpreter-2580ns ± 1%577ns ± 1%~(p=0.589 n=6+6)
ParseArray-27.74ms ± 1%7.70ms ± 2%~(p=0.731 n=6+7)
ParseDeploy/byte_array-211.6ms ± 1%11.9ms ± 7%~(p=0.101 n=6+7)
ParseDeploy/decode_hex-21.26ms ± 4%1.26ms ± 4%~(p=0.805 n=7+7)
ParseFungibleToken/With_memory_metering-2183µs ± 1%183µs ± 1%~(p=1.000 n=7+7)
ParseFungibleToken/Without_memory_metering-2143µs ± 1%144µs ± 2%~(p=0.710 n=7+7)
ParseInfix-27.09µs ± 4%7.03µs ± 2%~(p=0.402 n=7+7)
QualifiedIdentifierCreation/One_level-22.42ns ± 0%2.57ns ±19%~(p=0.332 n=7+7)
QualifiedIdentifierCreation/Three_levels-2120ns ± 0%121ns ± 5%~(p=0.730 n=5+7)
RuntimeFungibleTokenTransfer-2538µs ±46%575µs ±21%~(p=0.710 n=7+7)
RuntimeResourceDictionaryValues-25.24ms ± 1%5.20ms ± 2%~(p=0.295 n=7+6)
RuntimeScriptNoop-219.1µs ±30%16.5µs ±26%~(p=0.097 n=7+7)
SuperTypeInference/arrays-2301ns ± 1%301ns ± 1%~(p=0.828 n=7+7)
SuperTypeInference/composites-2132ns ± 0%131ns ± 0%−0.70%(p=0.001 n=7+7)
SuperTypeInference/integers-296.0ns ± 0%95.5ns ± 0%−0.54%(p=0.001 n=7+6)
ValueIsSubtypeOfSemaType-286.9ns ± 1%83.0ns ± 1%−4.49%(p=0.001 n=7+6)
 
alloc/opdelta
CheckContractInterfaceFungibleTokenConformance-248.9kB ± 0%48.9kB ± 0%~(all equal)
ContractInterfaceFungibleToken-223.0kB ± 0%23.0kB ± 0%~(all equal)
InterpretRecursionFib-21.00MB ± 0%1.00MB ± 0%~(p=0.385 n=7+6)
NewInterpreter/new_interpreter-2768B ± 0%768B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-2200B ± 0%200B ± 0%~(all equal)
ParseArray-22.73MB ± 3%2.76MB ± 5%~(p=0.902 n=7+7)
ParseDeploy/byte_array-24.16MB ± 3%4.26MB ± 0%~(p=0.181 n=7+6)
ParseDeploy/decode_hex-2214kB ± 0%214kB ± 0%~(p=0.674 n=7+6)
ParseFungibleToken/With_memory_metering-229.4kB ± 0%29.4kB ± 0%~(p=0.633 n=7+7)
ParseFungibleToken/Without_memory_metering-229.4kB ± 0%29.4kB ± 0%~(p=0.633 n=7+7)
ParseInfix-21.92kB ± 0%1.92kB ± 0%~(p=0.760 n=7+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
RuntimeFungibleTokenTransfer-2101kB ± 1%102kB ± 1%~(p=0.053 n=7+7)
RuntimeResourceDictionaryValues-22.28MB ± 0%2.28MB ± 0%~(p=0.318 n=7+7)
RuntimeScriptNoop-26.85kB ± 1%6.85kB ± 1%~(p=0.827 n=7+7)
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-2797 ± 0%797 ± 0%~(all equal)
ContractInterfaceFungibleToken-2363 ± 0%363 ± 0%~(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=0.417 n=7+6)
ParseDeploy/byte_array-289.4k ± 0%89.4k ± 0%~(p=0.592 n=7+7)
ParseDeploy/decode_hex-264.0 ± 0%64.0 ± 0%~(all equal)
ParseFungibleToken/With_memory_metering-2779 ± 0%779 ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-2779 ± 0%779 ± 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)
RuntimeFungibleTokenTransfer-21.97k ± 0%1.98k ± 0%+0.28%(p=0.001 n=7+6)
RuntimeResourceDictionaryValues-236.9k ± 0%36.9k ± 0%~(p=0.515 n=7+5)
RuntimeScriptNoop-298.0 ± 0%98.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)
 

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work! 👏

@dsainati1 dsainati1 merged commit 7d88676 into feature/attachments Dec 12, 2022
@dsainati1 dsainati1 deleted the sainati/attachment-declaration-interpreting branch December 12, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants