Proposal for generics-based Setter #201

medge-mailgun · 2024-09-18T16:06:32Z

Using generics, it should now be possible to achieve setter functionality without reflection. If accepted, original methods will be marked as Deprecated but remain for existing usage.

A major change here is that slices and maps do not work with the setter.Default() method. setter.Slice() has been added thus far. setter.Map() has proven interesting to do effectively and usage within Mailgun has not yet been found.

Edit: while this was not done for performance reasons, rather as an attempt to simplify implementation code, here are the comparisons between approaches:

Running on an M3 Macbook yields the following benchmark results:

go test -bench="BenchmarkSetter*" -benchmem -count=10 -tags holster_test_mode -run="^$" ./...

goos: darwin
goarch: arm64
pkg: github.com/mailgun/holster/v4/setter
BenchmarkSetterNew-12    	1000000000	         0.9060 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9524 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9062 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9053 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9059 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9024 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9057 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9052 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9054 ns/op.       0 B/op	       0 allocs/op
BenchmarkSetterNew-12    	1000000000	         0.9056 ns/op	       0 B/op	       0 allocs/op

BenchmarkSetter-12       	210037180	         5.647 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	213309236	         5.626 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	210853106	         5.648 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	211399341	         5.635 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	213350558	         5.690 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	213267204	         5.671 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	210416016	         5.672 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	210588644	         5.678 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	210472189	         5.790 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12       	212123646	         5.683 ns/op	       0 B/op	       0 allocs/op

Updated benchmarks with a Slice test added:

goos: darwin
goarch: arm64
pkg: github.com/mailgun/holster/v4/setter
BenchmarkSetterNew-12          	1000000000	         0.9063 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9057 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9063 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9061 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9064 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9054 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9047 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9048 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9064 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew-12          	1000000000	         0.9060 ns/op	       0 B/op	       0 allocs/op

BenchmarkSetter-12             	213260366	         5.625 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	212896548	         5.636 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	211280826	         5.614 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	212247787	         5.660 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	211494753	         5.642 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	211180790	         5.643 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	211073300	         5.649 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	211085336	         5.660 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	205125676	         5.782 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetter-12             	205552485	         5.777 ns/op	       0 B/op	       0 allocs/op

BenchmarkSetterNew_Slice-12    	415313892	         2.892 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	414445138	         2.890 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	414308188	         2.894 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	413750835	         2.897 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	414357903	         2.888 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	414068553	         2.893 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	414783637	         2.894 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	413121838	         2.910 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	414325414	         3.040 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetterNew_Slice-12    	414430587	         2.892 ns/op	       0 B/op	       0 allocs/op

BenchmarkSetter_Slice-12       	18901965	        62.86 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	18860769	        62.71 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	19149100	        62.84 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	19035922	        62.91 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	18961514	        62.92 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	18966596	        62.92 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	18814023	        63.76 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	18702063	        63.67 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	18443803	        63.64 ns/op	     112 B/op	       4 allocs/op
BenchmarkSetter_Slice-12       	18936728	        63.31 ns/op	     112 B/op	       4 allocs/op

thrawn01 · 2024-09-18T16:14:56Z

Setter was never intended to be used in hot code paths, but if you can achieve a performance improvement that would be very cool indeed. I forked this project for my own uses so I could use `SetDefault()` without any other external dependencies. If successful, it might also be advantageous to make the change there as well. https://github.com/kapetan-io/tackle

…

On Wed, Sep 18, 2024, 11:06 AM medge ***@***.***> wrote: Using generics, it should now be possible to achieve setter functionality without reflection. If accepted: I can, either, add this to a /setter/v2 package or upgrade the existing methods. Running on an M3 Macbook yields the following benchmark results: go test -bench="BenchmarkSetter*" -benchmem -count=10 -tags holster_test_mode -run="^$" ./... goos: darwin goarch: arm64 pkg: github.com/mailgun/holster/v4/setter BenchmarkSetterNew-12 <http://github.com/mailgun/holster/v4/setterBenchmarkSetterNew-12> 1000000000 0.9060 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9524 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9062 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9053 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9059 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9024 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9057 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9052 ns/op 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9054 ns/op. 0 B/op 0 allocs/op BenchmarkSetterNew-12 1000000000 0.9056 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 210037180 5.647 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 213309236 5.626 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 210853106 5.648 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 211399341 5.635 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 213350558 5.690 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 213267204 5.671 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 210416016 5.672 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 210588644 5.678 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 210472189 5.790 ns/op 0 B/op 0 allocs/op BenchmarkSetter-12 212123646 5.683 ns/op 0 B/op 0 allocs/op ------------------------------ You can view, comment on, or merge this pull request online at: #201 Commit Summary - dd8f762 <dd8f762> Proposal for generics-based Setter File Changes (2 files <https://github.com/mailgun/holster/pull/201/files>) - *M* setter/setter.go <https://github.com/mailgun/holster/pull/201/files#diff-b82061095521d8fc5246208728c2f4c313fb1039f8764482ba7bfcea9406e948> (15) - *M* setter/setter_test.go <https://github.com/mailgun/holster/pull/201/files#diff-8c99c56fa1594873d5407e231bfabc28ff5c33583dedb3843a77a23ba97c597a> (109) Patch Links: - https://github.com/mailgun/holster/pull/201.patch - https://github.com/mailgun/holster/pull/201.diff — Reply to this email directly, view it on GitHub <#201>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACMWMZVVNPMWZSWTEO2GDZXGQJ3AVCNFSM6AAAAABOOBEJVGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGUZTIMJUGE3TSOA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

medge-mailgun · 2024-09-18T16:26:04Z

@thrawn01 good to know! The change was mostly intended to be a code reduction change vs. a performance improvement, for sure. The benchmark was added purely for curiosity's sake 🙏
Caveat, of course, is that this doesn't seem to work for Slices and Maps, so may be less of a code reduction than originally hoped 😓

vtopc · 2024-09-19T16:23:50Z

setter/setter.go

+
+// IsZeroNew compares the given value to its Golang-specified zero value.
+// It works for any type T that satisfies comparable.
+func IsZeroNew[T comparable](value T) bool {


should it be exportable?

Good question. The original method is both exported and used in a few libraries so that was the motivation, though I'm still pondering if it's needed 🤔

It is better to make it non-exportable and who needs could copy it as it's pretty small:

A little copying is better than a little dependency.

https://go-proverbs.github.io/

Well, I just don't like this name 😄

Bahaha I don't like it either :D I'll leave the existing method (IsZero) exported and un-export this one, given the restricted use 👍

vtopc · 2024-09-20T15:08:40Z

setter/setter_test.go

+	assert.Equal(t, false, setter.IsZeroNew(thing))
+}
+
+// Not possible now given compiler warnings


In what Go version?

This is a good point - this change would not work pre-Go generics, as far as I am aware. So Go 1.18+?

We are not running it in Go 1.18 - https://github.com/mailgun/holster/blob/master/.github/workflows/test.yaml#L19-L20

Are you seeing these warnings on your local env?

That's correct. And that's why these tests are commented out. It's not possible to do what these tests did previously with the new methods. That's what its demonstrating 👀

Mind you, the name of the methods changed. I can fix that

And to be explicit: this test was previously trying to assign an int to a string, which you technically could do in the original method. The generics form doesn't allow that. It's a compilation error

And to be explicit: this test was previously trying to assign an int to a string, which you technically could do in the original method. The generics form doesn't allow that. It's a compilation error

I got it now.

Yeah, we don't need them at all, please remove the commented code. https://kentcdodds.com/blog/please-dont-commit-commented-out-code

Yep, all that will be removed once we settle on the PR final form 👍 I just left them there as a note to reviewers!

…benchmark for slices

medge-mailgun force-pushed the setter-generics branch from e9bbfd4 to 450a971 Compare September 18, 2024 16:24

medge-mailgun force-pushed the setter-generics branch from 70c7b5a to e107827 Compare September 19, 2024 16:00

vtopc reviewed Sep 19, 2024

View reviewed changes

medge-mailgun force-pushed the setter-generics branch from e107827 to cdff537 Compare September 19, 2024 20:25

vtopc reviewed Sep 20, 2024

View reviewed changes

medge-mailgun added 3 commits September 20, 2024 15:00

Proposal for generics-based Setter

9f4a117

Fix docker compose CLI usage

55cb412

Add dedicated DefaultSlice function given comparable constraint. Add …

c99bf26

…benchmark for slices

medge-mailgun force-pushed the setter-generics branch from cdff537 to c99bf26 Compare September 20, 2024 19:00

Fix commented test methods

1dc5951

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for generics-based Setter #201

Proposal for generics-based Setter #201

medge-mailgun commented Sep 18, 2024 •

edited

Loading

thrawn01 commented Sep 18, 2024 via email

medge-mailgun commented Sep 18, 2024 •

edited

Loading

vtopc Sep 19, 2024

medge-mailgun Sep 19, 2024

vtopc Sep 20, 2024 •

edited

Loading

medge-mailgun Sep 20, 2024

vtopc Sep 20, 2024

medge-mailgun Sep 20, 2024

vtopc Sep 23, 2024

medge-mailgun Sep 23, 2024

medge-mailgun Sep 23, 2024

vtopc Sep 23, 2024

medge-mailgun Sep 23, 2024

Proposal for generics-based Setter #201

Are you sure you want to change the base?

Proposal for generics-based Setter #201

Conversation

medge-mailgun commented Sep 18, 2024 • edited Loading

thrawn01 commented Sep 18, 2024 via email

medge-mailgun commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtopc Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medge-mailgun commented Sep 18, 2024 •

edited

Loading

medge-mailgun commented Sep 18, 2024 •

edited

Loading

vtopc Sep 20, 2024 •

edited

Loading