Skip to content

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented May 4, 2025

The New* methods unnecessarily allocate. If the user needs a pointer, they can convert, but most users probably only need it by value.

@pav-kv pav-kv requested a review from a team as a code owner May 4, 2025 01:26
@pav-kv pav-kv requested a review from mhutchinson May 4, 2025 01:26
@pav-kv pav-kv force-pushed the compact-no-alloc branch from f2bcebe to 30d1576 Compare May 4, 2025 01:35
The New* methods unnecessarily allocate. If the user needs a pointer,
they can convert, but most users only need it by value.
@pav-kv pav-kv force-pushed the compact-no-alloc branch from 30d1576 to 007cda6 Compare May 4, 2025 01:40
@pav-kv
Copy link
Contributor Author

pav-kv commented May 4, 2025

For reviewers: I don't feel strongly about this PR, but thought I'd throw it in for consideration. Been using this package for a side-project, and figured why not remove an allocation.

The current state is safer / avoids some footguns by design. So it's conceivable that it should stay this way.

Copy link

codecov bot commented May 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.18%. Comparing base (4ebea17) to head (007cda6).
Report is 74 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #174       +/-   ##
===========================================
- Coverage   89.33%   58.18%   -31.16%     
===========================================
  Files           7        8        +1     
  Lines         497      782      +285     
===========================================
+ Hits          444      455       +11     
- Misses         48      322      +274     
  Partials        5        5               

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@mhutchinson mhutchinson left a comment

Choose a reason for hiding this comment

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

Hey Pavel I intend to review this by the end of the week. I've just returned from vacation and need to get up to speed before deciding whether this subtle variation is worth the extra API method or not. Thanks for sending it!

@pav-kv
Copy link
Contributor Author

pav-kv commented May 9, 2025

@mhutchinson nvm, I think Go optimizes the allocation out if we say cr := *factory.NewRange(...):

diff --git a/compact/range_test.go b/compact/range_test.go
index 53bf58a..998c0cc 100644
--- a/compact/range_test.go
+++ b/compact/range_test.go
@@ -578,10 +578,10 @@ func TestDecomposePow2(t *testing.T) {
 
 func BenchmarkNewRange(b *testing.B) {
        b.ReportAllocs()
-       var cr *compact.Range
+       var cr compact.Range
        for range b.N {
-               // BenchmarkNewRange-10  69894982  17.13 ns/op  48 B/op  1 allocs/op
-               cr = factory.NewEmptyRange(10)
+               // BenchmarkNewRange-10  1000000000  0.3165 ns/op  0 B/op  0 allocs/op
+               cr = *factory.NewEmptyRange(10)
        }
        _ = cr
 }

@pav-kv
Copy link
Contributor Author

pav-kv commented May 9, 2025

I'll remove the API changes, and reduce this PR just to a bit of commenting in Range around the pointer/value stuff, since it still applies. Some time this/next week :)

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