Skip to content

Conversation

costela
Copy link
Contributor

@costela costela commented Aug 13, 2025

About the changes

This PR does a couple of smaller things:

  • first we make the benchmarks a bit more realistic by actually adding features (makes IsEnabled less of a noop)
  • drop usage of reflect by making every generic
  • initialize allConstraints to its proper size to avoid multiple slice resizings

The changes above bring a measurable performance improvement:

goos: linux
goarch: amd64
pkg: github.com/Unleash/unleash-go-sdk/v5
cpu: 12th Gen Intel(R) Core(TM) i7-1250U
                           │  0-old.out  │          3-makeslice.out           │
                           │   sec/op    │   sec/op     vs base               │
FeatureToggleEvaluation-12   1.546µ ± 4%   1.455µ ± 4%  -5.86% (p=0.002 n=10)

                           │  0-old.out   │           3-makeslice.out           │
                           │     B/op     │     B/op      vs base               │
FeatureToggleEvaluation-12   1.383Ki ± 0%   1.328Ki ± 0%  -3.95% (p=0.000 n=10)

                           │ 0-old.out  │          3-makeslice.out           │
                           │ allocs/op  │ allocs/op   vs base                │
FeatureToggleEvaluation-12   13.00 ± 0%   11.00 ± 0%  -15.38% (p=0.000 n=10)

Additionally, we also do a couple of minor internal cleanups:

  • switch to math/rand/v2
  • modernize with golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize
  • move more code to using stdlib
  • do not ignore errors in storage.Reset

Important files

The biggest impact is caused by the change in utils.go, in the every function.
Followed by the slice initialization in client.go.

In general, since the PR has multiple parts, it may be easier to check each commit individually.

Discussion points

Since there's a lot going on in the PR, one could argue for splitting it into smaller ones.

@costela costela marked this pull request as ready for review August 13, 2025 14:30
@costela costela changed the title modernize-and-minor-performance-improvements Modernize and minor performance improvements Aug 13, 2025
@FredrikOseberg FredrikOseberg moved this from New to Investigating in Issues and PRs Aug 20, 2025
Copy link

stale bot commented Sep 12, 2025

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 12, 2025
We implement the storage to force the benchmark to
cover more code, going into slower code paths.
This commit is just the result of calling:
"golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize -fix"
@costela costela force-pushed the leo.antunes/modernize-and-minor-performance-improvements branch from 4c74fe1 to 4912388 Compare September 16, 2025 08:12
@costela
Copy link
Contributor Author

costela commented Sep 16, 2025

This is not stale 🙄

@gastonfournier gastonfournier moved this from Investigating to New in Issues and PRs Sep 24, 2025
@FredrikOseberg
Copy link
Contributor

@costela Sorry for the delay, but we have had a lot going on as well as going on a company offsite last week that we do once a year. I haven't had time to sit down and contemplate the changes here in detail. My main concern is whether or not we need to do a major release with the interface changes.

I hope I can take a better look at this next week.

@FredrikOseberg
Copy link
Contributor

@costela

Could you lessen my workload a bit by highlighting the key implications of these changes?

@costela
Copy link
Contributor Author

costela commented Sep 30, 2025

@FredrikOseberg Sure!

  • The first commit (26727f4) just gives us a more realistic baseline for benchmarking, using more of the IsEnabled code paths. No user-facing implications. ✔️
  • The second (8d83f29) is just the result of calling modernize, so it should have no implications ✔️
  • The third (aa9397d) just removes dead code and replaces custom rounding in tests with math.Round. So hopefully no implications either ✔️
  • The fourth (91a442f) is a bit more interesting: it switches to defer for some unlocking since it has been pretty on-par performance-wise for a long time, and is marginally safer. It also replaces math/rand with math/rand/v2, just out of force of habit 🙈 It should have no sensible implications for consumers, but may require a bit more scrutiny. 🕵️
  • The fifth (9f05cd0) drops reflect usage in the internal every function. It causes a measurable performance improvement (see benchmarks in OP) and should have no side-effects, since it implements basically the same checks as before, just on compile time.
  • The sixth (9becd32) could also use some scrutiny: we now bubble any errors during storage reset. I believe this is safer than ignoring the error, but I may be overlooking some side-effects. 🕵️
  • And lastly the seventh (4912388) is a pretty straightforward micro-optimization and just pre-allocates the allConstraints slice to the worst-case larges capacity. Seems pretty safe? ✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

3 participants