-
Notifications
You must be signed in to change notification settings - Fork 3
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
Go 1.18 generics for lazylru #14
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dangermike
force-pushed
the
dangermike/NOTICKET/go1.18
branch
11 times, most recently
from
December 29, 2021 09:03
b6af442
to
bce4aeb
Compare
gnagel
approved these changes
Mar 21, 2022
dangermike
force-pushed
the
dangermike/NOTICKET/go1.18
branch
2 times, most recently
from
March 21, 2022 23:18
ef574e6
to
0624280
Compare
The current version of the cache uses `string` as the key and `interface{}` as the value. That fits the use case for which it was designed, but it is not as flexible as it could be. Go 1.18 generics created an opportunity to do change that. The [container/heap](https://pkg.go.dev/container/heap) in the standard library doesn't support generics. I'm sure it will at some point, but for now, the source code was copied from the standard libary and made generic. The `pq` and `lazylru` components were copied into a subpackage, `generic`. While the internals of the library (and especially the tests) are littered with type annotations, the external interface is pretty clean. Previously, the cache would be used like so: ```go // import "github.com/TriggerMail/lazylru" lru := lazylru.New(maxItems, ttl) lru.Set("key", "value") if v, ok := lru.Get("key"); ok { vstr, ok := v.(string) if !ok { panic("something terrible has happened") } fmt.Println(vstr) } ``` The new version is a bit cleaner: ```go // import "github.com/TriggerMail/lazylru/generic" lru := lazylru.NewT[string, string](maxItems, ttl) lru.Set("key", "value") if v, ok := lru.Get("key"); ok { fmt.Println(v) } ``` It's expected that the cache is going to be created at the start of a program and accessed many times, so the real win is the lack of casting on the `Get`. It is easy to put in a value when you mean a pointer or a pointer when you mean a value, but the generic version prevents that problem. The `panic` in the sample code above is a maybe overkill, but the caller is likely to do _something_ to deal with type. There is a performance impact to the casting, but it doesn't appear to be huge. In terms of caching performance, there was an improvement in all cases. I tested the original, interface-based implementation as well as a generic implementation of [string, interface{}] to mimic the interface type as closely as possible and a generic implementation of `[string, int]` to see what the improvement would be. Tests were run on an Apple Macbook Pro M1. An excerpt of the benchmarch is listed below: ```text 1% W, 99% R 99% W, 1% R ------------------------- ------------ ------------ interface-based 60.94 ns/op 107.80 ns/op generic[string,interface] 54.21 ns/op 87.76 ns/op generic[string,int] 53.24 ns/op 93.80 ns/op ``` * Separate interface and generic versions to allow the consumer to select the generic as a version * Make testing work under go 1.18 * golang:rc-buster image * go fmt * installing go-junit-report properly * adding test-results to .gitignore * Building with Earthly to make life easier on myself * Using revive rather than golangci-lint because golangci-lint doesn't work with go 1.18 yet * Publishing coverage results to coveralls * On interface (top-level) only because goveralls doesn't like submodules * README badges for coverage
* passing the build number from buildkite to Earthly * passing build number as jobname to coveralls * adding git above generic so coveralls figures its stuff out
Including some nice struct align fixes
dangermike
force-pushed
the
dangermike/NOTICKET/go1.18
branch
from
March 22, 2022 00:04
0624280
to
13c6b9c
Compare
ghost
mentioned this pull request
Jul 30, 2022
Merged
ghost
mentioned this pull request
Feb 22, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current version of the cache uses
string
as the key andinterface{}
as the value. That fits the use case for which it was designed, but it is not as flexible as it could be. Go 1.18 generics created an opportunity to do change that.The container/heap in the standard library doesn't support generics. I'm sure it will at some point, but for now, the source code was copied from the standard libary and made generic. The
pq
andlazylru
components were copied into a subpackage,generic
. While the internals of the library (and especially the tests) are littered with type annotations, the external interface is pretty clean. Previously, the cache would be used like so:The new version is a bit cleaner:
It's expected that the cache is going to be created at the start of a program and accessed many times, so the real win is the lack of casting on the
Get
. It is easy to put in a value when you mean a pointer or a pointer when you mean a value, but the generic version prevents that problem. Thepanic
in the sample code above is a maybe overkill, but the caller is likely to do something to deal with type. There is a performance impact to the casting, but it doesn't appear to be huge.In terms of caching performance, there was an improvement in all cases. I tested the original, interface-based implementation as well as a generic implementation of [string, interface{}] to mimic the interface type as closely as possible and a generic implementation of
[string, int]
to see what the improvement would be. Tests were run on an Apple Macbook Pro M1. An excerpt of the benchmarch is listed below: