Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

modified freelist_hmap/hashmapGetFreePageIDs with better performance #419

Merged
merged 1 commit into from
Mar 15, 2023
Merged

modified freelist_hmap/hashmapGetFreePageIDs with better performance #419

merged 1 commit into from
Mar 15, 2023

Conversation

cenkalti
Copy link
Member

Resurrected #219

@cenkalti cenkalti mentioned this pull request Mar 14, 2023
Closed
@ahrtr ahrtr added this to the v1.4.0 milestone Mar 14, 2023
serathius
serathius reviewed Mar 14, 2023
View reviewed changes
freelist_test.go
@@ -434,3 +434,27 @@ func newTestFreelist() *freelist {

return newFreelist(freelistType)
}

func Test_freelist_hashmapGetFreePageIDs(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please implement a microbenchmark an use benchstat to show performance improvement. Article shows how properly use it.

Sorry for being a stickler, but from my experience it's easy to make mistakes when optimizing and we need to standardize the process for writing performance improvement PRs.

Copy link
Member Author

@cenkalti cenkalti Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

benchmark                                       old ns/op     new ns/op     delta
Benchmark_freelist_hashmapGetFreePageIDs-12     21804519      181847        -99.17%

benchmark                                       old allocs     new allocs     delta
Benchmark_freelist_hashmapGetFreePageIDs-12     2              3              +50.00%

benchmark                                       old bytes     new bytes     delta
Benchmark_freelist_hashmapGetFreePageIDs-12     802886        812828        +1.24%

@cenkalti cenkalti force-pushed the freelist_hmap branch 2 times, most recently from cce63f8 to f6ccf09 Compare March 15, 2023 03:04
@cenkalti
modified freelist_hmap/hashmapGetFreePageIDs with better performance
c9c264c 
Signed-off-by: Cenk Alti <cenkalti@gmail.com>
ahrtr
ahrtr approved these changes Mar 15, 2023
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @cenkalti

serathius
serathius approved these changes Mar 15, 2023
View reviewed changes
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

99% time reduction for 1 alloc. Nice!

@ahrtr ahrtr merged commit eb468cb into etcd-io:master Mar 15, 2023
@ahrtr ahrtr mentioned this pull request Apr 14, 2023
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@serathius serathius serathius approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
v1.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cenkalti @serathius @ahrtr