Improve block comment parsing #2068

turbolent · 2022-10-13T23:18:46Z

Work towards #1884

Description

Simplify from trampoline to simple for loop (No idea what I was thinking when writing this initially ...)
Avoid string-building in block comment
Improve error for EOF. It was missing a position and had a confusing error message

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 · 2022-10-13T23:20:02Z

runtime/parser/parser.go

+			if ok && options.parseDocStrings {
+				commentEndOffset := endToken.EndPos.Offset
+
+				contentWithPrefix := p.tokens.Input()[commentStartOffset : commentEndOffset-1]


No need to rebuild the block comment content with a string builder, we can get the source from the token stream

codecov · 2022-10-13T23:28:39Z

Codecov Report

Merging #2068 (a50557a) into master (6d7549e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2068      +/-   ##
==========================================
+ Coverage   77.65%   77.66%   +0.01%     
==========================================
  Files         303      302       -1     
  Lines       63163    63158       -5     
==========================================
+ Hits        49047    49051       +4     
+ Misses      12370    12360      -10     
- Partials     1746     1747       +1

Flag	Coverage Δ
unittests	`77.66% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
runtime/parser/comment.go	`100.00% <100.00%> (+29.26%)`	⬆️
runtime/parser/expression.go	`90.32% <100.00%> (ø)`
runtime/parser/parser.go	`90.34% <100.00%> (+0.08%)`	⬆️
runtime/stdlib/account.go	`85.83% <0.00%> (-0.12%)`	⬇️
runtime/interpreter/interpreter.go	`89.05% <0.00%> (-0.01%)`	⬇️
runtime/interpreter/globalvariables.go	`62.50% <0.00%> (ø)`
runtime/interpreter/value.go	`70.76% <0.00%> (+<0.01%)`	⬆️
runtime/interpreter/storage.go	`70.63% <0.00%> (+1.58%)`	⬆️

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

github-actions · 2022-10-13T23:33:43Z

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 6d7549e
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.txt	new.txt
	time/op		delta
CheckContractInterfaceFungibleTokenConformance-2	119µs ± 1%	120µs ± 2%	~	(p=0.445 n=6+7)
ContractInterfaceFungibleToken-2	41.4µs ± 2%	42.1µs ± 1%	+1.60%	(p=0.010 n=7+5)
InterpretRecursionFib-2	2.38ms ± 1%	2.45ms ± 6%	+2.94%	(p=0.035 n=6+7)
NewInterpreter/new_interpreter-2	1.21µs ± 1%	1.16µs ± 1%	−4.15%	(p=0.001 n=7+6)
NewInterpreter/new_sub-interpreter-2	680ns ± 2%	617ns ± 1%	−9.35%	(p=0.001 n=6+7)
ParseArray-2	8.47ms ± 9%	8.20ms ± 1%	~	(p=0.234 n=7+6)
ParseDeploy/byte_array-2	12.5ms ± 2%	12.6ms ± 2%	~	(p=0.456 n=7+7)
ParseDeploy/decode_hex-2	1.21ms ± 1%	1.22ms ± 1%	~	(p=0.836 n=6+7)
ParseFungibleToken/With_memory_metering-2	191µs ± 1%	192µs ± 3%	~	(p=1.000 n=7+7)
ParseFungibleToken/Without_memory_metering-2	153µs ± 2%	152µs ± 4%	~	(p=0.318 n=7+7)
ParseInfix-2	7.20µs ± 1%	7.31µs ± 5%	~	(p=0.702 n=6+7)
QualifiedIdentifierCreation/One_level-2	2.35ns ± 0%	2.35ns ± 1%	~	(p=0.493 n=6+7)
QualifiedIdentifierCreation/Three_levels-2	140ns ± 2%	144ns ± 3%	+3.04%	(p=0.008 n=7+7)
RuntimeFungibleTokenTransfer-2	676µs ±22%	731µs ± 2%	~	(p=0.234 n=7+6)
RuntimeResourceDictionaryValues-2	5.21ms ± 3%	5.25ms ± 3%	~	(p=0.209 n=7+7)
RuntimeScriptNoop-2	18.3µs ±51%	19.5µs ±23%	~	(p=0.535 n=7+7)
ValueIsSubtypeOfSemaType-2	93.0ns ± 1%	93.8ns ± 1%	~	(p=0.102 n=6+6)

	alloc/op		delta
CheckContractInterfaceFungibleTokenConformance-2	51.0kB ± 0%	51.0kB ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	24.2kB ± 0%	24.2kB ± 0%	~	(p=1.000 n=7+7)
InterpretRecursionFib-2	1.00MB ± 0%	1.00MB ± 0%	~	(p=0.567 n=6+6)
NewInterpreter/new_interpreter-2	848B ± 0%	800B ± 0%	−5.66%	(p=0.001 n=7+7)
NewInterpreter/new_sub-interpreter-2	264B ± 0%	216B ± 0%	−18.18%	(p=0.001 n=7+7)
ParseArray-2	2.76MB ± 4%	2.73MB ± 3%	~	(p=0.445 n=6+7)
ParseDeploy/byte_array-2	4.21MB ± 3%	4.18MB ± 3%	~	(p=0.876 n=7+7)
ParseDeploy/decode_hex-2	214kB ± 0%	214kB ± 0%	~	(p=0.727 n=7+7)
ParseFungibleToken/With_memory_metering-2	29.4kB ± 0%	29.4kB ± 0%	−0.23%	(p=0.001 n=7+7)
ParseFungibleToken/Without_memory_metering-2	29.4kB ± 0%	29.4kB ± 0%	−0.20%	(p=0.000 n=6+7)
ParseInfix-2	1.93kB ± 0%	1.93kB ± 0%	~	(p=0.603 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00B	0.00B	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	64.0B ± 0%	64.0B ± 0%	~	(all equal)
RuntimeFungibleTokenTransfer-2	103kB ± 1%	103kB ± 1%	~	(p=0.805 n=7+7)
RuntimeResourceDictionaryValues-2	2.27MB ± 0%	2.27MB ± 0%	−0.33%	(p=0.007 n=7+7)
RuntimeScriptNoop-2	8.90kB ± 2%	8.84kB ± 0%	~	(p=0.259 n=7+7)
ValueIsSubtypeOfSemaType-2	48.0B ± 0%	48.0B ± 0%	~	(all equal)

	allocs/op		delta
CheckContractInterfaceFungibleTokenConformance-2	915 ± 0%	915 ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	436 ± 0%	436 ± 0%	~	(all equal)
InterpretRecursionFib-2	18.9k ± 0%	18.9k ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	14.0 ± 0%	13.0 ± 0%	−7.14%	(p=0.001 n=7+7)
NewInterpreter/new_sub-interpreter-2	5.00 ± 0%	4.00 ± 0%	−20.00%	(p=0.001 n=7+7)
ParseArray-2	69.6k ± 0%	69.6k ± 0%	~	(p=0.140 n=7+6)
ParseDeploy/byte_array-2	104k ± 0%	104k ± 0%	~	(p=0.245 n=7+6)
ParseDeploy/decode_hex-2	75.0 ± 0%	75.0 ± 0%	~	(all equal)
ParseFungibleToken/With_memory_metering-2	1.00k ± 0%	0.99k ± 0%	−0.10%	(p=0.001 n=7+7)
ParseFungibleToken/Without_memory_metering-2	1.00k ± 0%	0.99k ± 0%	−0.10%	(p=0.001 n=7+7)
ParseInfix-2	60.0 ± 0%	60.0 ± 0%	~	(all equal)
QualifiedIdentifierCreation/One_level-2	0.00	0.00	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	2.00 ± 0%	2.00 ± 0%	~	(all equal)
RuntimeFungibleTokenTransfer-2	2.06k ± 0%	2.06k ± 0%	−0.05%	(p=0.001 n=7+7)
RuntimeResourceDictionaryValues-2	36.9k ± 0%	36.9k ± 0%	~	(p=0.449 n=7+7)
RuntimeScriptNoop-2	145 ± 0%	145 ± 0%	~	(all equal)
ValueIsSubtypeOfSemaType-2	1.00 ± 0%	1.00 ± 0%	~	(all equal)

runtime/parser/comment.go

turbolent added 2 commits October 13, 2022 15:33

improve block comment parsing, add tests

ff0b502

optimize docstring extraction

5cd0d15

turbolent added the Performance label Oct 13, 2022

turbolent requested a review from a team October 13, 2022 23:18

turbolent requested review from SupunS and dsainati1 as code owners October 13, 2022 23:18

turbolent self-assigned this Oct 13, 2022

turbolent commented Oct 13, 2022

View reviewed changes

dreamsmasher approved these changes Oct 13, 2022

View reviewed changes

test invalid block comment token stream

80d2049

SupunS approved these changes Oct 14, 2022

View reviewed changes

runtime/parser/comment.go Outdated Show resolved Hide resolved

refactor exit

a50557a

turbolent merged commit 6990e2d into master Oct 14, 2022

turbolent deleted the bastian/optimize-comment-parsing branch October 14, 2022 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve block comment parsing #2068

Improve block comment parsing #2068

turbolent commented Oct 13, 2022

turbolent Oct 13, 2022

codecov bot commented Oct 13, 2022 •

edited

Loading

github-actions bot commented Oct 13, 2022 •

edited

Loading

Improve block comment parsing #2068

Improve block comment parsing #2068

Conversation

turbolent commented Oct 13, 2022

Description

turbolent Oct 13, 2022

Choose a reason for hiding this comment

codecov bot commented Oct 13, 2022 • edited Loading

Codecov Report

github-actions bot commented Oct 13, 2022 • edited Loading

Cadence Benchstat comparison

codecov bot commented Oct 13, 2022 •

edited

Loading

github-actions bot commented Oct 13, 2022 •

edited

Loading