Introduce benchmark for the smallIntegerValueCache

[darkdrag00nv2]

Work towards #2871

Description

Introduces benchmark for the smallIntegerValueCache.

Results on my machine

Without Cache

BenchmarkIntegerCreationWithoutCache-16    	      91	  15024692 ns/op	12304062 B/op	  971404 allocs/op

Command: go test -benchmem -run=^$ -bench ^BenchmarkIntegerCreationWithoutCache$ github.com/onflow/cadence/runtime/interpreter

With Cache

BenchmarkSmallIntegerValueCache-16    	      86	  13857062 ns/op	 7401608 B/op	  767457 allocs/op

Command: go test -benchmem -run=^$ -bench ^BenchmarkSmallIntegerValueCache$ github.com/onflow/cadence/runtime/interpreter

---

• 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

[SupunS]

Can you please also post the results for the two benchmarks?

[turbolent]

Thank you for adding the benchmarks!

Something seems off in the cached version, either in the benchmark or the reporting for it:

BenchmarkSmallIntegerValueCache-10         456	   2576811 ns/op	      46 B/op	       0 allocs/op
BenchmarkIntegerCreationWithoutCache-10    368	   3275893 ns/op	 4089900 B/op	  204002 allocs/op

How come the cached version has 0 allocations? I'd expect it to allocate less, but at least something

[turbolent]

Using the value, e.g. by calling Sprintf, the results look a bit better and show the cache saves quite some allocations and isn't slower 👍

[darkdrag00nv2]

@SupunS Added the results on my machine as well in the description. The cache does seem to help.

@turbolent Had to use Sprintf. runtime.KeepAlive was also leading to 0 allocs as you pointed our earlier.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7ab492f) 79.63% compared to head (3836f60) 79.63%.

Additional details and impacted files

@@                 Coverage Diff                 @@
##           feature/range-type    #2936   +/-   ##
===================================================
  Coverage               79.63%   79.63%           
===================================================
  Files                     336      336           
  Lines                   79754    79754           
===================================================
  Hits                    63511    63511           
  Misses                  13875    13875           
  Partials                 2368     2368           

Flag	Coverage Δ
unittests	79.63% <100.00%> (ø)

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

Files	Coverage Δ
runtime/interpreter/integer.go	96.61% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bluesign]

Very nice to have cache for small integers. 👏

How come the cached version has 0 allocations? I'd expect it to allocate less, but at least something

It is optimized to use register there probably.

[darkdrag00nv2]

@turbolent @SupunS Shall we merge this? The results look fine and I think it'll be nice to check-in the benchmark.

[SupunS]

Looks good!
My only concern is the sprintf() call is affecting the benchmark numbers quite a bit (i.e: sprintf is more expensive than the number creation). But seems there is no other way to avoid optimizing away.

[turbolent]

I don't think we need to merge this in actually, the benchmarks were just a means to see if the caching approach is worthwhile. We usually have benchmarks checked in for things we do not want to regress performance on, as well as like Supun pointed out, the benchmarks are tricky to get right / are not ideal in the current form.

It was still very valuable to have these benchmarks to give us data to make decisions 🙏
We'll keep using the number cache 👍