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

Add HasTag method to OopsError #52

Merged
merged 3 commits into from
Mar 5, 2025
Merged

Conversation

zzzLobster
Copy link
Contributor

This is to implement proposal #51

@samber
Copy link
Owner

samber commented Mar 4, 2025

Hey @zzzLobster

Thanks for this first contrib.

Can you add it to the readme please?

Did you evaluate to change o.tags from a slice to a map? I would be faster for HasTag, but maybe too slow for other helpers.

@zzzLobster
Copy link
Contributor Author

Hey @samber

Can you add it to the readme please?

Sure, I'll do that.

Did you evaluate to change o.tags from a slice to a map? I would be faster for HasTag, but maybe too slow for other helpers.

I did not, to avoid unnecessary changes. However, I think that's a good idea, so I'll check this option.

Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.71%. Comparing base (eeb65a7) to head (11f4dbe).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   76.17%   76.71%   +0.54%     
==========================================
  Files          13       13              
  Lines        1024     1035      +11     
==========================================
+ Hits          780      794      +14     
+ Misses        212      210       -2     
+ Partials       32       31       -1     
Flag Coverage Δ
unittests 76.71% <100.00%> (+0.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zzzLobster
Copy link
Contributor Author

Did you evaluate to change o.tags from a slice to a map? I would be faster for HasTag, but maybe too slow for other helpers.

TLDR: it does not worth to replace slice with map in this case.

Implementation with map:
f81c1cf
I've tried these 3 test cases:
ff08225

And run 2 variants:

  1. const errorNum = 10 - stack of 10 errors
    const tagsPerError = 10 - 10 tags for each error
    results are pretty much the same on this scale
  2. const errorNum = 50
    const tagsPerError = 100
    On this scale (insane one, IMO) HasTags() works faster, but Tags() as well as instantiation of errors with all the tags works slower, sometimes significantly
-- tags as slice, variant 1
go test -benchmem -count=3 -bench=.
goos: darwin
goarch: arm64
pkg: github.com/samber/oops
cpu: Apple M1 Pro
BenchmarkCreateErrorsWithTags-8            10017            119628 ns/op           56208 B/op        841 allocs/op
BenchmarkCreateErrorsWithTags-8            10000            116857 ns/op           56208 B/op        841 allocs/op
BenchmarkCreateErrorsWithTags-8             9493            116186 ns/op           56208 B/op        841 allocs/op
BenchmarkGetTags-8                        120414              9779 ns/op           13464 B/op         19 allocs/op
BenchmarkGetTags-8                        115333              9613 ns/op           13464 B/op         19 allocs/op
BenchmarkGetTags-8                        122136              9243 ns/op           13464 B/op         19 allocs/op
BenchmarkHasTag-8                         228723              5191 ns/op            5824 B/op         22 allocs/op
BenchmarkHasTag-8                         242548              5251 ns/op            5824 B/op         22 allocs/op
BenchmarkHasTag-8                         230253              5184 ns/op            5824 B/op         22 allocs/op
-- tags as slice, variant 2
go test -benchmem -count=3 -bench=.
goos: darwin
goarch: arm64
pkg: github.com/samber/oops
cpu: Apple M1 Pro
BenchmarkCreateErrorsWithTags-8             1828            629100 ns/op          361492 B/op       4201 allocs/op
BenchmarkCreateErrorsWithTags-8             2005            596663 ns/op          361496 B/op       4201 allocs/op
BenchmarkCreateErrorsWithTags-8             1944            602877 ns/op          361492 B/op       4201 allocs/op
BenchmarkGetTags-8                          3998            288678 ns/op          517498 B/op         63 allocs/op
BenchmarkGetTags-8                          3855            288675 ns/op          517498 B/op         63 allocs/op
BenchmarkGetTags-8                          3997            297040 ns/op          517498 B/op         63 allocs/op
BenchmarkHasTag-8                          24432             48337 ns/op           28864 B/op        102 allocs/op
BenchmarkHasTag-8                          25134             48675 ns/op           28864 B/op        102 allocs/op
BenchmarkHasTag-8                          24434             47938 ns/op           28864 B/op        102 allocs/op
-- tags as map, variant 1
go test -benchmem -count=3 -bench=.
goos: darwin
goarch: arm64
pkg: github.com/samber/oops
cpu: Apple M1 Pro
BenchmarkCreateErrorsWithTags-8             7268            140579 ns/op           59440 B/op        861 allocs/op
BenchmarkCreateErrorsWithTags-8             9007            128058 ns/op           59438 B/op        861 allocs/op
BenchmarkCreateErrorsWithTags-8             8281            133277 ns/op           59439 B/op        861 allocs/op
BenchmarkGetTags-8                         85934             13683 ns/op           10376 B/op         20 allocs/op
BenchmarkGetTags-8                         99601             13267 ns/op           10375 B/op         20 allocs/op
BenchmarkGetTags-8                         93613             12337 ns/op           10376 B/op         20 allocs/op
BenchmarkHasTag-8                         231360              5158 ns/op            5824 B/op         22 allocs/op
BenchmarkHasTag-8                         185398              5855 ns/op            5824 B/op         22 allocs/op
BenchmarkHasTag-8                         220131              5347 ns/op            5824 B/op         22 allocs/op
-- tags as map, variant 2
go test -benchmem -count=3 -bench=.
goos: darwin
goarch: arm64
pkg: github.com/samber/oops
cpu: Apple M1 Pro
BenchmarkCreateErrorsWithTags-8             1171           1080981 ns/op          552351 B/op       4673 allocs/op
BenchmarkCreateErrorsWithTags-8             1309            933342 ns/op          552261 B/op       4673 allocs/op
BenchmarkCreateErrorsWithTags-8             1363            888331 ns/op          552350 B/op       4673 allocs/op
BenchmarkGetTags-8                          2350            535821 ns/op          438888 B/op        164 allocs/op
BenchmarkGetTags-8                          2335            527145 ns/op          438880 B/op        163 allocs/op
BenchmarkGetTags-8                          2397            549266 ns/op          438800 B/op        163 allocs/op
BenchmarkHasTag-8                          39244             33631 ns/op           28864 B/op        102 allocs/op
BenchmarkHasTag-8                          39927             27897 ns/op           28864 B/op        102 allocs/op
BenchmarkHasTag-8                          44702             27582 ns/op           28864 B/op        102 allocs/op

@zzzLobster
Copy link
Contributor Author

@samber
I've added a line to readme, please let me know if that's better to describe it in some other place as well

@samber
Copy link
Owner

samber commented Mar 5, 2025

thanks @zzzLobster for your benchmarks ! ✌️

let's merge ;)

@samber samber merged commit 3f82677 into samber:main Mar 5, 2025
7 checks passed
@zzzLobster zzzLobster deleted the feat/has_tag branch March 5, 2025 16:13
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.

2 participants