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

Track inner values of resource-kinded optionals #1630

Merged
merged 4 commits into from
May 9, 2022

Conversation

dsainati1
Copy link
Contributor

@dsainati1 dsainati1 commented May 6, 2022

Closes #1539

Description

With the change in #1303, it is no longer possible to obtain a reference to an optional value, as all such references are automatically transformed into optional references. However, these references were not properly tracked by the logic added in #1344, meaning that the linked issue was still possible despite the cause seemingly being removed. This adds proper tracking for the inner values of resource-kinded optionals.


  • 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

@dsainati1 dsainati1 requested a review from turbolent as a code owner May 6, 2022 18:43
@dsainati1 dsainati1 self-assigned this May 6, 2022
@dsainati1 dsainati1 requested a review from SupunS as a code owner May 6, 2022 18:43
Base automatically changed from optional-resource-reference to master May 6, 2022 23:42
@github-actions
Copy link

github-actions bot commented May 9, 2022

Cadence Benchstat comparison

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

Results

old.txtnew.txt
time/opdelta
Transfer-2105ns ± 3%109ns ± 4%+3.83%(p=0.024 n=7+7)
RuntimeFungibleTokenTransfer-21.63ms ±23%1.58ms ±23%~(p=1.000 n=7+7)
RuntimeResourceDictionaryValues-27.71ms ± 3%7.85ms ± 8%~(p=0.445 n=6+7)
ParseFungibleToken-2231µs ± 2%232µs ± 3%~(p=0.805 n=7+7)
ParseArray-215.2ms ± 7%15.2ms ± 3%~(p=0.620 n=7+7)
ParseDeploy/byte_array-223.6ms ± 4%23.8ms ± 5%~(p=0.620 n=7+7)
ParseDeploy/decode_hex-21.50ms ± 1%1.51ms ± 2%~(p=0.259 n=7+7)
ParseInfix-210.1µs ± 2%10.3µs ± 2%~(p=0.073 n=7+7)
QualifiedIdentifierCreation/One_level-23.47ns ± 1%3.48ns ± 1%~(p=0.314 n=7+6)
QualifiedIdentifierCreation/Three_levels-2175ns ± 1%177ns ± 3%~(p=0.091 n=7+7)
CheckContractInterfaceFungibleTokenConformance-2174µs ± 1%177µs ± 3%~(p=0.053 n=7+7)
ContractInterfaceFungibleToken-249.1µs ± 2%49.7µs ± 3%~(p=0.209 n=7+7)
NewInterpreter/new_interpreter-21.39µs ± 2%1.39µs ± 2%~(p=0.865 n=7+6)
NewInterpreter/new_sub-interpreter-22.73µs ± 1%2.73µs ± 2%~(p=0.773 n=7+7)
InterpretRecursionFib-23.27ms ± 3%3.29ms ± 1%~(p=0.534 n=7+6)
 
alloc/opdelta
RuntimeFungibleTokenTransfer-2273kB ± 0%273kB ± 0%~(p=0.767 n=7+6)
RuntimeResourceDictionaryValues-22.25MB ± 0%2.25MB ± 0%~(p=0.435 n=7+7)
Transfer-248.0B ± 0%48.0B ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-266.3kB ± 0%66.3kB ± 0%~(all equal)
ContractInterfaceFungibleToken-226.7kB ± 0%26.7kB ± 0%~(all equal)
NewInterpreter/new_interpreter-2848B ± 0%848B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-21.34kB ± 0%1.34kB ± 0%~(all equal)
InterpretRecursionFib-21.14MB ± 0%1.14MB ± 0%~(p=0.462 n=7+7)
 
allocs/opdelta
RuntimeFungibleTokenTransfer-24.58k ± 0%4.58k ± 0%~(p=0.516 n=7+7)
RuntimeResourceDictionaryValues-237.6k ± 0%37.6k ± 0%~(p=0.478 n=7+7)
Transfer-21.00 ± 0%1.00 ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
ContractInterfaceFungibleToken-2460 ± 0%460 ± 0%~(all equal)
NewInterpreter/new_interpreter-213.0 ± 0%13.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-240.0 ± 0%40.0 ± 0%~(all equal)
InterpretRecursionFib-223.8k ± 0%23.8k ± 0%~(all equal)
 

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #1630 (b7d1a82) into master (7b77432) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1630      +/-   ##
==========================================
- Coverage   74.76%   74.76%   -0.01%     
==========================================
  Files         288      288              
  Lines       55382    55386       +4     
==========================================
+ Hits        41406    41408       +2     
- Misses      12481    12483       +2     
  Partials     1495     1495              
Flag Coverage Δ
unittests 74.76% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
runtime/interpreter/interpreter_expression.go 84.70% <100.00%> (+0.08%) ⬆️
runtime/interpreter/errors.go 22.11% <0.00%> (-1.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b77432...b7d1a82. Read the comment docs.

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!

For the other resource-kinded objects (composites, dictionaries, and arrays) we could track via their dedicated storage ID (as they are mutable, they are stored in separate slabs).
However, optionals (SomeValue) are potentially inlined in storage, so do not have a dedicated storage ID.

I thought at first this is an issue, but as you showed, tracking the potentially resource-kinded inner value of the optional works fine, nice 👍

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.

Accessing resource reference causes "missing value for member" error if resource has been moved
2 participants