Skip to content
This repository was archived by the owner on Oct 25, 2024. It is now read-only.

Improve performance of token compilation #12

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Conversation

estensen
Copy link
Contributor

@estensen estensen commented Nov 30, 2022

📝 Summary

  • Grow builder before using it to avoid unnecessary memory allocation and copy.
  • Use hex.EncodeToString to convert bytes to hex

📚 References

strings.Builder is backed by a slice, so Grow() pre-allocates this slice
https://cs.opensource.google/go/go/+/refs/tags/go1.19.3:src/strings/builder.go;l=15

TODO: Add benchmark


@metachris metachris requested a review from Ruteri December 2, 2022 10:15
@Ruteri
Copy link
Collaborator

Ruteri commented Dec 2, 2022

Is this also the case upstream @estensen ? Maybe it'd make sense to contribute there too

@Ruteri
Copy link
Collaborator

Ruteri commented Dec 2, 2022

Correct me if I'm wrong
bin.WriteString(fmt.Sprintf("%x", v)) can write more than a byte, so the string is not guaranteed to not reallocate right?
Do you have any data as to how often it's the case either way?

@estensen
Copy link
Contributor Author

estensen commented Dec 2, 2022

Yeah, planning to upstream this as well :)

As far as I understand, it doesn't matter if you add a byte (converted to string) or a longer string. The string builder will append one item per iteration and it's the number of items that causes re-allocation of slices and not the length of the items.

@estensen estensen changed the title Pre-grow string builder Improve performance of token compilation Dec 2, 2022
@estensen
Copy link
Contributor Author

estensen commented Dec 2, 2022

I'm getting my feet wet here. How many tokens are typically compiled?

Running benchmarks locally growing the string builder is faster from around 25 items.
https://github.com/estensen/benchmarks/blob/master/strings/concatenation.go
https://github.com/estensen/benchmarks/blob/master/strings/concatenation_test.go

// 10
BenchmarkConcatString/concat_string-12         	 3155166	       368.5 ns/op	     600 B/op	       9 allocs/op
BenchmarkConcatString/concat_buffer-12         	 7609831	       157.1 ns/op	     304 B/op	       3 allocs/op
BenchmarkConcatString/concat_builder-12        	 8009695	       150.8 ns/op	     240 B/op	       4 allocs/op
BenchmarkConcatString/concat_grown_builder-12  	 6620098	       182.3 ns/op	     376 B/op	       5 allocs/op
// 25
BenchmarkConcatString/concat_string-12         	  994122	      1190 ns/op	    3400 B/op	      24 allocs/op
BenchmarkConcatString/concat_buffer-12         	 3759690	       316.4 ns/op	     704 B/op	       4 allocs/op
BenchmarkConcatString/concat_builder-12        	 4358062	       272.9 ns/op	     496 B/op	       5 allocs/op
BenchmarkConcatString/concat_grown_builder-12  	 5165503	       232.3 ns/op	     480 B/op	       4 allocs/op
// 100
BenchmarkConcatString/concat_string-12         	  113984	     10431 ns/op	   53480 B/op	      99 allocs/op
BenchmarkConcatString/concat_buffer-12         	 1000000	      1097 ns/op	    3008 B/op	       6 allocs/op
BenchmarkConcatString/concat_builder-12        	 1000000	      1048 ns/op	    3312 B/op	       8 allocs/op
BenchmarkConcatString/concat_grown_builder-12  	 1405492	       859.5 ns/op	    2656 B/op	       5 allocs/op

@estensen
Copy link
Contributor Author

estensen commented Dec 2, 2022

hex.EncodeToString was faster than fmt.Sprintf for all the benchmarks I did

https://github.com/estensen/benchmarks/blob/master/encoding/encoding.go
https://github.com/estensen/benchmarks/blob/master/encoding/encoding_test.go

BenchmarkByteToHex/encode_bytes_to_hex_string_with_fmt.Sprintf_for_1_byte-12         	        12061374	        86.88 ns/op	      26 B/op	       2 allocs/op
BenchmarkByteToHex/encode_bytes_to_hex_string_with_hex.EncodeToString_for_1_byte-12         	84560934	        14.46 ns/op	       2 B/op	       1 allocs/op
BenchmarkByteToHex/encode_bytes_to_hex_string_with_fmt.Sprintf_for_5_bytes-12        	        11170132	       106.2 ns/op	      40 B/op	       2 allocs/op
BenchmarkByteToHex/encode_bytes_to_hex_string_with_hex.EncodeToString_for_5_bytes-12 	        42864435	        27.07 ns/op	      16 B/op	       1 allocs/op
BenchmarkByteToHex/encode_bytes_to_hex_string_with_fmt.Sprintf_for_10_bytes-12       	        10061223	       115.1 ns/op	      48 B/op	       2 allocs/op
BenchmarkByteToHex/encode_bytes_to_hex_string_with_hex.EncodeToString_for_10_bytes-12         	33870447	        34.29 ns/op	      24 B/op	       1 allocs/op
BenchmarkByteToHex/encode_bytes_to_hex_string_with_fmt.Sprintf_for_50_bytes-12                	 5790674	       204.7 ns/op	     136 B/op	       2 allocs/op
BenchmarkByteToHex/encode_bytes_to_hex_string_with_hex.EncodeToString_for_50_bytes-12         	11064771	       108.3 ns/op	     224 B/op	       2 allocs/op

@Ruteri
Copy link
Collaborator

Ruteri commented Dec 2, 2022

Sounds good.
Thanks for doing detailed benchmarks and your contribution!
I'll merge once tested.

@Ruteri Ruteri merged commit c216aca into flashbots:main Dec 6, 2022
@estensen estensen deleted the pre-grow branch December 6, 2022 17:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants