crossbeam-skiplist: allows lookup to be customized.#1132
crossbeam-skiplist: allows lookup to be customized.#1132taiki-e merged 13 commits intocrossbeam-rs:masterfrom
crossbeam-skiplist: allows lookup to be customized.#1132Conversation
|
I ran across this recently too, unfortunately the skiplist sits in my hot path, so the allocation is unfortunate. Using this PR increased performance by like 10% (for the entire read path, of which the skiplist is only a part of): BeforeYou can see that most of the time stems from allocating and drop_in_place of the Key Arc... After@al8n Very nice change! |
|
Is there anything that can be done to move this forward? I guess this could be a breaking change, but crossbeam is < 1.0 anyway - I've been thinking of forking crossbeam-skiplist, just so I can get this change into my code base... |
|
Thank for the PR. To be honest, I suspect that the “efficiency” issue here can actually be resolved in some cases by using the appropriate key wrappers... |
I have already published a forked version |
|
It seems that @cuviper has tried something similar to this before (indexmap-rs/indexmap#253 (comment)). The approach there does not seem to have type inference issues unlike this PR? (or am I missed something?) |
I finished the changes about flipping |
|
Nice! Could you revert the turbofish addition on crossbeam-skiplist/tests/map.rs and crossbeam-skiplist/tests/set.rs as well? https://github.com/crossbeam-rs/crossbeam/pull/1132/files#diff-75f4f67227e1c745e030de5d756b0cc4cb11ccd273aac4bd1cf8277d4c7abc30 |
Finished. |
Could I merge this PR? |
|
The first version of this PR was a breaking change because it required turbofish in some places, but I guess the current version will actually not cause the breakage that would be considered a breaking change. |
|
@taiki-e Can this change be published (possibly as pre-release) to crates? |
|
New release is blocked by what to do with #1158. |
That doesn't seem to change the API though, seems more like a refactor to me. |
|
I think such a change is a breaking change when done after this change is released. See tokio-rs/bytes#479 (comment) for more. |
Actually, I am fine with keeping I will also give a look how |


Hi, this PR solves #1133, which changes the lookup API from
to
The
equivalentis the crate used byindexmap.I would like to suggest this change because when using
SkipMap, I find the lookup API are limited byBorrowtrait. e.g., withBorrowtrait, this example cannot be implemented as we cannot implementimpl<'a> core::borrow::Borrow<FooRef<'a>> for Foobecause of Rust's rule.After change to
Q: ?Sized + Ord + Comparable<K>, just like howindexmapdo in its lookup API, the above example can be implemented.This feature is quite important when using some zero-copy deserialization frameworks like
rkyv's types withSkipMapto avoid the allocation.Closes #1133