Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2: use generics to enforce type invariants #43

Merged
merged 4 commits into from
Dec 16, 2022
Merged

v2: use generics to enforce type invariants #43

merged 4 commits into from
Dec 16, 2022

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Jul 7, 2022

This PR introduces a v2 of go-immutable-radix, centered on utilizing generics.

The major version bump is necessary because although the package is now generic, the exported type signatures have changed to be generic; rather than assuming interface{} in places.

  • sets minimum Go version to 1.18
  • module is bumped to github.com/hashicorp/go-immutable-radix/v2
  • CI is swapped from Circle to GitHub Actions
  • Exported types and their methods are now generic
  • minor code lint fixes

Closes #42
Closes #36

r := iradix.New[string]() // e.g.

note: if this gets merged, be sure to tag with v2.0.0

Adding some benchmarks (via this harness). I suspect in practice the performance difference is negligible - the differences here could just be luck with GC going one way or the other.

Work/go/iradix-bench 24s
➜ go run main.go 
Scenario   Type        Size           V1           V2        Delta      Pct
 insertion byte        1000   1.697583ms   1.140482ms   -557.101µs  -32.82%
 insertion byte       10000  11.958884ms   9.357029ms  -2.601855ms  -21.76%
 insertion byte      100000 105.298741ms 110.699253ms   5.400512ms    5.13%
 insertion byte     1000000 741.490742ms 733.548246ms  -7.942496ms   -1.07%
 insertion employee    1000   1.231465ms   1.193493ms    -37.972µs   -3.08%
 insertion employee   10000  13.319666ms  14.263141ms    943.475µs    7.08%
 insertion employee  100000 110.649808ms  98.866248ms  -11.78356ms  -10.65%
 insertion employee 1000000 748.178939ms 681.968127ms -66.210812ms   -8.85%
 insertion int64       1000    1.33419ms   1.126917ms   -207.273µs  -15.54%
 insertion int64      10000  10.679749ms  10.884198ms    204.449µs    1.91%
 insertion int64     100000 113.991954ms 113.522559ms   -469.395µs   -0.41%
 insertion int64    1000000 754.626542ms 723.907751ms -30.718791ms   -4.07%
 insertion string      1000    993.762µs    559.075µs   -434.687µs  -43.74%
 insertion string     10000   12.36633ms  11.791997ms   -574.333µs   -4.64%
 insertion string    100000  97.246453ms  99.044017ms   1.797564ms    1.85%
 insertion string   1000000 697.957946ms 656.948516ms  -41.00943ms   -5.88%
   iterate byte        1000     17.052µs     12.504µs     -4.548µs  -26.67%
   iterate byte       10000    312.355µs    192.366µs   -119.989µs  -38.41%
   iterate byte      100000   6.272204ms   6.321789ms     49.585µs    0.79%
   iterate byte     1000000  62.505377ms   60.94112ms  -1.564257ms   -2.50%
   iterate employee    1000      10.46µs     12.905µs      2.445µs   23.37%
   iterate employee   10000    292.166µs    306.554µs     14.388µs    4.92%
   iterate employee  100000   5.937789ms   6.243939ms     306.15µs    5.16%
   iterate employee 1000000  64.140997ms   80.17852ms  16.037523ms   25.00%
   iterate int64       1000     16.511µs     14.368µs     -2.143µs  -12.98%
   iterate int64      10000     262.46µs    126.381µs   -136.079µs  -51.85%
   iterate int64     100000   6.378087ms   6.439172ms     61.085µs    0.96%
   iterate int64    1000000  60.678524ms  60.375718ms   -302.806µs   -0.50%
   iterate string      1000      7.134µs      6.763µs       -371ns   -5.20%
   iterate string     10000     194.05µs     105.27µs     -88.78µs  -45.75%
   iterate string    100000   6.041565ms   6.171173ms    129.608µs    2.15%
   iterate string   1000000  64.068893ms  64.387791ms    318.898µs    0.50%

@shoenig shoenig marked this pull request as ready for review July 7, 2022 02:52
.github/ci.yaml Outdated Show resolved Hide resolved
schmichael added a commit to hashicorp/nomad that referenced this pull request Jul 12, 2022
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I ported Nomad's acl package to it, and it works just like you'd expect.

hashicorp/nomad#13721

(Let's get at least one more stakeholder review before merging though since it's such a foundational package.)

@shoenig shoenig force-pushed the use-generics branch 6 times, most recently from 59e77f0 to 3f7e9ee Compare July 13, 2022 13:53
go.mod Outdated

require (
github.com/hashicorp/go-uuid v1.0.0
github.com/hashicorp/golang-lru v0.5.0
github.com/hashicorp/golang-lru v0.5.4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we update this dependency to the v2 version of golang-lru? (this one introduces generics for the cache as well)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, done!

@lthibault
Copy link

Is this still being worked on? Anything I can do to help?

Copy link

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done a lot with Go generics yet, so not sure you want to trust my review, but this all seems sensible.

Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 👏 !
A thought out of the scope of this PR but now that we are adding a v2 could we also make the key generic?

I took a stab into that a while back and the challenge is to find an elegant way of extracting the dependencies to the bytes package. We use the following:

  • bytes.HasPrefix
  • bytes.Compare
  • bytes.Equal

Not sure if it's useful though 🤔

@lthibault
Copy link

LGTM.

I did notice that this comment has not been addressed, though. It's a trivial change, so I suggest including it.

@shoenig
Copy link
Member Author

shoenig commented Dec 16, 2022

@dhiaayachi indeed I went down that rabbit hole for a bit myself - the increase in complexity is hard to justify without a clear benefit; but if that becomes apparent one day there can always be a v3 🙂

@shoenig shoenig merged commit c57308e into master Dec 16, 2022
@lthibault
Copy link

Thanks for the quick turn-around, everyone! 🎉

shoenig pushed a commit to hashicorp/nomad that referenced this pull request Dec 16, 2022
schmichael added a commit to hashicorp/nomad that referenced this pull request Dec 19, 2022
* Migrate acls to generics

See hashicorp/go-immutable-radix#43

* deps: fixup go.mod formatting

Co-authored-by: Seth Hoenig <shoenig@duck.com>
@abhijitWakchaure
Copy link

@shoenig After the use-generics branch is merged I'm not able to build my code which is using https://github.com/hashicorp/go-metrics which refers this code. I think generics are used in this repo incorrectly. Here is the error I'm getting:

cannot use generic type iradix.Tree[T any] without instantiation
cannot infer T (github.com/hashicorp/go-immutable-radix/iradix.go:32:10

@kisunji
Copy link
Contributor

kisunji commented Jul 6, 2023

@shoenig After the use-generics branch is merged I'm not able to build my code which is using https://github.com/hashicorp/go-metrics which refers this code. I think generics are used in this repo incorrectly. Here is the error I'm getting:

cannot use generic type iradix.Tree[T any] without instantiation
cannot infer T (github.com/hashicorp/go-immutable-radix/iradix.go:32:10

If you are using the v2 module, you need to pass a type like

r := iradix.New[string]()
r := iradix.New[any]()
// etc...

The github.com/hashicorp/go-metrics doesn't have github.com/hashicorp/go-immutable-radix/v2 as a dependency so it's a bit puzzling that you're getting that error. Could you provide a snippet of the code you're trying to compile?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Q: Support for Generics? please modify example in README
8 participants