NotificationsYou must be signed in to change notification settings
Fork34
Star291

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Proposal for generics-based Setter#201

Draft

medge-mailgun wants to merge4 commits intomaster

base:master

Choose a base branch

fromsetter-generics

Draft

Proposal for generics-based Setter#201

medge-mailgun wants to merge4 commits intomasterfromsetter-generics

Conversation

Copy link

Contributor

medge-mailgun commentedSep 18, 2024•
edited
Loading

Using generics, it should now be possible to achievesetter 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 thesetter.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: darwingoarch: arm64pkg: github.com/mailgun/holster/v4/setterBenchmarkSetterNew-12    1000000000         0.9060 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9524 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9062 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9053 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9059 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9024 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9057 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9052 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9054 ns/op.       0 B/op       0 allocs/opBenchmarkSetterNew-12    1000000000         0.9056 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       210037180         5.647 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       213309236         5.626 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       210853106         5.648 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       211399341         5.635 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       213350558         5.690 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       213267204         5.671 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       210416016         5.672 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       210588644         5.678 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       210472189         5.790 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12       212123646         5.683 ns/op       0 B/op       0 allocs/op

Updated benchmarks with a Slice test added:

goos: darwingoarch: arm64pkg: github.com/mailgun/holster/v4/setterBenchmarkSetterNew-12          1000000000         0.9063 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9057 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9063 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9061 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9064 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9054 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9047 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9048 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9064 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew-12          1000000000         0.9060 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             213260366         5.625 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             212896548         5.636 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             211280826         5.614 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             212247787         5.660 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             211494753         5.642 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             211180790         5.643 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             211073300         5.649 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             211085336         5.660 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             205125676         5.782 ns/op       0 B/op       0 allocs/opBenchmarkSetter-12             205552485         5.777 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    415313892         2.892 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    414445138         2.890 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    414308188         2.894 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    413750835         2.897 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    414357903         2.888 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    414068553         2.893 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    414783637         2.894 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    413121838         2.910 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    414325414         3.040 ns/op       0 B/op       0 allocs/opBenchmarkSetterNew_Slice-12    414430587         2.892 ns/op       0 B/op       0 allocs/opBenchmarkSetter_Slice-12       18901965        62.86 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       18860769        62.71 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       19149100        62.84 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       19035922        62.91 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       18961514        62.92 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       18966596        62.92 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       18814023        63.76 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       18702063        63.67 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       18443803        63.64 ns/op     112 B/op       4 allocs/opBenchmarkSetter_Slice-12       18936728        63.31 ns/op     112 B/op       4 allocs/op

Copy link

Contributor

thrawn01 commentedSep 18, 2024 via email

Setter was never intended to be used in hot code paths, but if you canachieve a performance improvement that would be very cool indeed.I forked this project for my own uses so I could use `SetDefault()` withoutany other external dependencies. If successful, it might also beadvantageous 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 force-pushed thesetter-generics branch frome9bbfd4 to450a971Compare

September 18, 2024 16:24

Copy link

ContributorAuthor

medge-mailgun commentedSep 18, 2024•
edited
Loading

@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 😓

medge-mailgun force-pushed thesetter-generics branch from70c7b5a toe107827Compare

September 19, 2024 16:00

vtopc reviewed

Sep 19, 2024

View reviewed changes

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 {

Copy link

Contributor

vtopcSep 19, 2024

Choose a reason for hiding this comment

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

should it be exportable?

Copy link

ContributorAuthor

medge-mailgunSep 19, 2024

Choose a reason for hiding this comment

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

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 🤔

Copy link

Contributor

vtopcSep 20, 2024•
edited
Loading

Choose a reason for hiding this comment

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

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 😄

Copy link

ContributorAuthor

medge-mailgunSep 20, 2024

Choose a reason for hiding this comment

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

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 👍

medge-mailgun force-pushed thesetter-generics branch frome107827 tocdff537Compare

September 19, 2024 20:25

vtopc reviewed

Sep 20, 2024

View reviewed changes

setter/setter_test.go

		assert.Equal(t, false, setter.IsZeroNew(thing))
		}

		// Not possible now given compiler warnings

Copy link

Contributor

vtopcSep 20, 2024

Choose a reason for hiding this comment

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

In what Go version?

Copy link

ContributorAuthor

medge-mailgunSep 20, 2024

Choose a reason for hiding this comment

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

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

Copy link

Contributor

vtopcSep 23, 2024

Choose a reason for hiding this comment

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

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?

Copy link

ContributorAuthor

medge-mailgunSep 23, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

medge-mailgunSep 23, 2024

Choose a reason for hiding this comment

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

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

Copy link

Contributor

vtopcSep 23, 2024

Choose a reason for hiding this comment

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

And to be explicit: this test was previously trying to assign an int to a string, which youtechnically 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

Copy link

ContributorAuthor

medge-mailgunSep 23, 2024

Choose a reason for hiding this comment

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

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

medge-mailgun added3 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 thesetter-generics branch fromcdff537 toc99bf26Compare

September 20, 2024 19:00

Fix commented test methods

1dc5951

Labels

None yet

3 participants

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for generics-based Setter#201

Are you sure you want to change the base?

Proposal for generics-based Setter#201

Conversation

medge-mailgun commentedSep 18, 2024•
edited
Loading

thrawn01 commentedSep 18, 2024 via email

medge-mailgun commentedSep 18, 2024•
edited
Loading

vtopcSep 19, 2024

Choose a reason for hiding this comment

medge-mailgunSep 19, 2024

Choose a reason for hiding this comment

vtopcSep 20, 2024•
edited
Loading

Choose a reason for hiding this comment

medge-mailgunSep 20, 2024

Choose a reason for hiding this comment

vtopcSep 20, 2024

Choose a reason for hiding this comment

medge-mailgunSep 20, 2024

Choose a reason for hiding this comment

vtopcSep 23, 2024

Choose a reason for hiding this comment

medge-mailgunSep 23, 2024

Choose a reason for hiding this comment

medge-mailgunSep 23, 2024

Choose a reason for hiding this comment

vtopcSep 23, 2024

Choose a reason for hiding this comment

medge-mailgunSep 23, 2024

Choose a reason for hiding this comment

Movatterモバイル変換

Proposal for generics-based Setter#201

Are you sure you want to change the base?

Proposal for generics-based Setter#201

Conversation

medge-mailgun commentedSep 18, 2024• editedLoading

thrawn01 commentedSep 18, 2024 via email

medge-mailgun commentedSep 18, 2024• editedLoading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtopcSep 20, 2024• editedLoading

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 commentedSep 18, 2024•
edited
Loading

medge-mailgun commentedSep 18, 2024•
edited
Loading

vtopcSep 20, 2024•
edited
Loading