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 FindTagsAppend() #12

Merged
merged 2 commits into from
Sep 9, 2021
Merged

add FindTagsAppend() #12

merged 2 commits into from
Sep 9, 2021

Conversation

pforemski
Copy link
Contributor

@pforemski pforemski commented Sep 11, 2020

I suggest a FindTags() version that can minimize memory allocations, which is implemented in this PR.

The PR introduces FindTagsAppend(), which uses a destination slice instead of allocating it inside the function. The slice can be pre-allocated by the caller, and re-used between successive calls.

@wblakecaldwell
Copy link
Contributor

Thanks @pforemski. Good idea, I'll take a look. Sorry for the delay - I need to update my GitHub notifications :)

@pforemski
Copy link
Contributor Author

Any chance to merge this anytime soon? I'd love to go back using the mainstream repo ;)

@wblakecaldwell
Copy link
Contributor

wblakecaldwell commented Aug 31, 2021

@pforemski This is really great stuff - thank you so much. I took your idea and ran with it.

I'm not maintaining backwards compatibility. It'll require minor changes to update, but everyone's performance will improve because of it. I just converted the FindTags to your FindTagsAppend, updated FindDeepestTags and FindTagsWithFilter in the same vein, and added a buffer parameter to the Delete method.

While I was at it, I got rid of the unused error return values - as you noticed, they're all returning nil anyway.

Check this out - old code vs new on Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz in a VM:

BenchmarkFindTags-5                     	 3721257	       294.3 ns/op	     160 B/op	       6 allocs/op
BenchmarkFindTags-5                     	41273214	        28.12 ns/op	       0 B/op	       0 allocs/op

While I'm evaluating all of this, it's sitting here: https://github.com/kentik/patricia/tree/wbc-fewer-allocations, in this PR: #14

Have a look if you're interested, I'd love your feedback.

wblakecaldwell added a commit that referenced this pull request Sep 9, 2021
This incorporates the great work from https://github.com/pforemski, here: #12, then takes it a bit further. The method signatures have changed, so this isn't backwards compatible - the always-nil errors were removed. 

Changes:
- Removed always-nil error return values
- New methods that take slices to append to: `FindTagsAppend`, `FindTagsWithFilterAppend`, `FindDeepestTagsAppend`
- New `DeleteWithBuffer` now accepts a slice to use as a buffer
@wblakecaldwell wblakecaldwell merged commit b0db03a into kentik:master Sep 9, 2021
@wblakecaldwell
Copy link
Contributor

PR accepted with additions in this one #14

Thanks for the submission!

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.

None yet

2 participants