drop init time cost of apd from 0.530ms to 0.002ms

[mvdan]

(see commit messages - please do not squash)

[mvdan]

cc @josharian :)

[mvdan]

Friendly ping @nvanbenschoten :) Let me know if I should do anything else to help this along. Note that https://github.com/cockroachdb/apd/pull/139 caused conflicts so I dropped the CI changes from this PR for now.

[jordanlewis]

Thanks for the contribution, @mvdan!

Since this is a performance-related change, could you re-use or write a benchmark for this and put the results in the PR? Additionally, since there's new use of sync.Once, and this library does live in the critical hot-path for other systems, would you be able to run a benchdiff on any benchmarks in this package and include the results as well?

[mvdan]

I did provide benchmarks in the commit messages with https://github.com/mvdan/benchinit, which as far as I can tell is the best way we have for now to measure init time cost.

This package has a large number of benchmarks, is there any in particular that would be relevant here? Running all through benchstat is possible but rather costly.

[jordanlewis]

Ah, thanks, I didn't notice. Regarding which are relevant, I'm afraid I'm not familiar enough with this library to tell you off-hand. Looking at the diff, I see the sync.Once added to a few spots - would it be possible to trace those callsites up and find some relevant benchmarks to run that touches the changed code? If that's "literally everything" maybe just run a reasonable looking subset.

Really just looking to make sure we're not going to introduce any obvious regressions. Thank you!

[mvdan]

Unfortunately the benchmark suite is rather vast when including the sub-benchmarks, so trying to run them all with -count=6 -benchtime=1s literally timed out after ten minutes, so I gave up on that route.

I took one of the benchmarks which is simpler and clearly affected by one of the changes, and indeed there's some overhead by the added sync.Once:

                   │     old     │                 new                 │
                   │   sec/op    │   sec/op     vs base                │
NumDigitsLookup-16   8.376µ ± 1%   9.564µ ± 2%  +14.19% (p=0.000 n=10)

For some context, the changes here reduce the init time cost by about 0.530ms, and this NumDigits benchmark shows a slowdown of about 0.001ms. I'm not sure what your tradeoffs are in terms of init time cost versus func/method call cost, but I'd say half a millisecond at init time is way too much; most Go packages use 5-20 microseconds at init time at most.

[jordanlewis]

Thanks @mvdan! It doesn't matter to Cockroach Labs if this package takes half a millisecond to initialize because half an ms of initialization time is a rounding error for CockroachDB initialization time. By contrast, a 15% increase in the time it takes to do decimal operations could be a much bigger problem because the database might do a lot of those in memory for a math-intensive query - it's not inconceivable that a change like this could cause a 5% or larger regression in the overall time it takes to perform a database query for certain workloads.

As a result, I don't think this solution is of interest to our team, unfortunately.

What if an environment variable or other such input could tell the library to not perform the expensive initialization steps, and then expose the expensive initialization as a public function that could be used for use cases that set the env variable? Just brainstorming...

[mvdan]

Fair enough. Half a millisecond is still unfortunate for downstream CLI users, where every millisecond wasted in startup really does matter. But I don't know what to suggest here given that sync.Once and others add a bit of overhead, beyond requesting that you please give some love to init time cost - in a way that the tradeoff is acceptable to you.