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

Fix local token replay limit, add global replay limit #1663

Merged
merged 4 commits into from
May 24, 2022

Conversation

turbolent
Copy link
Member

@turbolent turbolent commented May 19, 2022

Description

The local replay was not correctly implemented:
An ambiguity does not just end when the buffered tokens are accepted, it also ends after replaying the buffered tokens and re-parsing using a different rule.
Explicitly state when an ambiguity starts and ends, and reset the local token replay limit.

In addition, add a global replay limit, as the local replay limit might not be sufficient in rejecting programs that are too ambiguous.

I parsed all Mainnet contracts using these changes and they all still parse.

TODO: Wrap errors in fatal errors, depends on #1660


  • 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

@turbolent turbolent requested a review from SupunS as a code owner May 19, 2022 20:47
@turbolent turbolent self-assigned this May 19, 2022
@turbolent turbolent requested a review from dsainati1 as a code owner May 19, 2022 20:47
@github-actions
Copy link

github-actions bot commented May 19, 2022

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit acc9090
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.

Results

old.txtnew.txt
time/opdelta
CheckContractInterfaceFungibleTokenConformance-2182µs ± 4%183µs ± 3%~(p=0.805 n=7+7)
ContractInterfaceFungibleToken-250.8µs ± 3%51.0µs ± 5%~(p=1.000 n=7+7)
InterpretRecursionFib-23.38ms ± 1%3.44ms ± 3%~(p=0.181 n=6+7)
NewInterpreter/new_interpreter-21.36µs ± 6%1.37µs ± 3%~(p=0.710 n=7+7)
NewInterpreter/new_sub-interpreter-22.76µs ± 6%2.79µs ± 3%~(p=1.000 n=7+7)
ParseArray-210.3ms ± 5%10.4ms ± 4%~(p=0.383 n=7+7)
ParseDeploy/byte_array-215.2ms ± 3%15.4ms ± 3%~(p=0.209 n=7+7)
ParseDeploy/decode_hex-21.48ms ± 6%1.49ms ± 2%~(p=0.902 n=7+7)
ParseFungibleToken/With_memory_metering-2257µs ± 4%262µs ± 2%~(p=0.128 n=7+7)
ParseFungibleToken/Without_memory_metering-2200µs ± 3%203µs ± 5%~(p=0.456 n=7+7)
ParseInfix-28.98µs ± 3%9.13µs ± 4%~(p=0.209 n=7+7)
QualifiedIdentifierCreation/One_level-23.44ns ± 5%3.47ns ± 5%~(p=0.833 n=7+7)
QualifiedIdentifierCreation/Three_levels-2173ns ± 4%180ns ± 3%+4.01%(p=0.014 n=7+6)
RuntimeFungibleTokenTransfer-21.59ms ±23%1.74ms ±22%~(p=0.318 n=7+7)
RuntimeResourceDictionaryValues-28.31ms ± 5%8.42ms ± 5%~(p=0.805 n=7+7)
Transfer-2114ns ± 5%113ns ± 3%~(p=0.929 n=7+7)
 
alloc/opdelta
CheckContractInterfaceFungibleTokenConformance-266.3kB ± 0%66.3kB ± 0%~(all equal)
ContractInterfaceFungibleToken-226.7kB ± 0%26.7kB ± 0%~(p=1.000 n=7+7)
InterpretRecursionFib-21.25MB ± 0%1.25MB ± 0%~(p=0.690 n=7+7)
NewInterpreter/new_interpreter-2872B ± 0%872B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-21.38kB ± 0%1.38kB ± 0%~(all equal)
ParseArray-22.93MB ± 2%2.96MB ± 2%~(p=0.383 n=7+7)
ParseDeploy/byte_array-24.26MB ± 4%4.30MB ± 3%~(p=0.128 n=7+7)
ParseDeploy/decode_hex-2213kB ± 0%213kB ± 0%~(p=0.664 n=7+6)
ParseFungibleToken/With_memory_metering-236.3kB ± 0%36.3kB ± 0%+0.04%(p=0.002 n=6+6)
ParseFungibleToken/Without_memory_metering-236.3kB ± 0%36.3kB ± 0%+0.07%(p=0.003 n=7+5)
ParseInfix-22.15kB ± 0%2.17kB ± 0%+0.75%(p=0.001 n=7+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
RuntimeFungibleTokenTransfer-2237kB ± 1%237kB ± 0%~(p=1.000 n=7+7)
RuntimeResourceDictionaryValues-22.24MB ± 0%2.24MB ± 0%~(p=0.710 n=7+7)
Transfer-248.0B ± 0%48.0B ± 0%~(all equal)
 
allocs/opdelta
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
ContractInterfaceFungibleToken-2460 ± 0%460 ± 0%~(all equal)
InterpretRecursionFib-223.8k ± 0%23.8k ± 0%~(all equal)
NewInterpreter/new_interpreter-212.0 ± 0%12.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-240.0 ± 0%40.0 ± 0%~(all equal)
ParseArray-270.0k ± 0%70.0k ± 0%~(p=1.000 n=7+7)
ParseDeploy/byte_array-2105k ± 0%105k ± 0%~(p=1.000 n=7+7)
ParseDeploy/decode_hex-279.0 ± 0%79.0 ± 0%~(all equal)
ParseFungibleToken/With_memory_metering-21.06k ± 0%1.06k ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-21.06k ± 0%1.06k ± 0%~(all equal)
ParseInfix-266.0 ± 0%66.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-24.58k ± 0%4.58k ± 0%~(p=0.464 n=7+7)
RuntimeResourceDictionaryValues-237.6k ± 0%37.6k ± 0%~(p=0.339 n=7+7)
Transfer-21.00 ± 0%1.00 ± 0%~(all equal)
 

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #1663 (cf0e7f9) into master (acc9090) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1663      +/-   ##
==========================================
+ Coverage   76.49%   76.51%   +0.01%     
==========================================
  Files         291      291              
  Lines       61022    61049      +27     
==========================================
+ Hits        46679    46709      +30     
+ Misses      12731    12727       -4     
- Partials     1612     1613       +1     
Flag Coverage Δ
unittests 76.51% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
runtime/parser2/expression.go 92.99% <100.00%> (+0.01%) ⬆️
runtime/parser2/parser.go 91.93% <100.00%> (+0.54%) ⬆️
runtime/parser2/type.go 89.93% <0.00%> (+0.47%) ⬆️

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 acc9090...cf0e7f9. Read the comment docs.

@turbolent turbolent force-pushed the bastian/local-and-global-replay-limit branch 2 times, most recently from 3bd7c69 to 2451ecd Compare May 24, 2022 21:48
@turbolent turbolent force-pushed the bastian/local-and-global-replay-limit branch from 2451ecd to cf0e7f9 Compare May 24, 2022 21:55
@turbolent turbolent merged commit b4a37c4 into master May 24, 2022
@turbolent turbolent deleted the bastian/local-and-global-replay-limit branch May 24, 2022 23:22
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.

4 participants