Skip to content

Add support for returning displaced entries for re-use.#22

Merged
mbrubeck merged 1 commit into
servo:masterfrom
Byron:master
Aug 22, 2021
Merged

Add support for returning displaced entries for re-use.#22
mbrubeck merged 1 commit into
servo:masterfrom
Byron:master

Conversation

@Byron

@Byron Byron commented Aug 22, 2021

Copy link
Copy Markdown
Contributor

I added this to be able to avoid allocations if the entry is allocating, In case of gitoxide this saves a couple of millions of allocations in favour of a few thousand re-allocs.

Performance wasn't measurably better in my case, just as if the allocator can already repurpose previously freed slabs of memory pretty well or at only a very low cost.

Since it doesn't seem to hurt providing this capability, I am creating this PR anyway in the hopes it does indeed make sense to some in this form or another.

Please let me know :).

@Byron

Byron commented Aug 22, 2021

Copy link
Copy Markdown
Contributor Author

Here is how the altered method can be used.

Byron added a commit to GitoxideLabs/gitoxide that referenced this pull request Aug 22, 2021
@mbrubeck

Copy link
Copy Markdown
Collaborator

Thanks, this is great. I don't think it needs to be an optional feature, though. You can use core::mem::replace in no_std crates. Also, changing the signature of a function when a feature is enabled could technically break other crates in the same dependency graph that depend on the type of the function.

I'll merge this PR, and then push a follow-up to make it always use the new method.

@mbrubeck mbrubeck merged commit a9e01cd into servo:master Aug 22, 2021
@mbrubeck

mbrubeck commented Aug 22, 2021

Copy link
Copy Markdown
Collaborator

@Byron

Byron commented Aug 23, 2021

Copy link
Copy Markdown
Contributor Author

Absolutely awesome, thank you!

Also thanks for removing the feature toggle, there is so much I don't know about no-std, but I will get there (probably when gitoxide goes WASM :) ).

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