-
Notifications
You must be signed in to change notification settings - Fork 142
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
RFC: Avoid double-indirection of Arc<Vec<u8>> by using Arc<[u8]> directly #803
RFC: Avoid double-indirection of Arc<Vec<u8>> by using Arc<[u8]> directly #803
Conversation
fcfe1d1
to
66e40e9
Compare
I'm not seeing a speed-up in https://github.com/cberner/redb/blob/master/benches/lmdb_benchmark.rs Is there a benchmark where you were able to measure a significant speed-up from this? |
This was not driven by benchmarks for me. I just stumbled over this while looking something else up in the code. I would say that there is usually never a reason to use Of course, the situation is a bit more complex here as the So if there are benchmark changes, I would expect them to be positive, but this was not my motivation. I would sort any performance changes under "distributed fat" as I would expect increased TLB/cache pressure from the additional indirection and possible heap fragmentation. |
66e40e9
to
0a63e55
Compare
Got it, I see. The first commit looks good then, but I don't want to add the Want to remove the second commit, and I'll merge the first? |
I did put that into a separate commit to make it easy to drop. I added it all because Otherwise, I'll do that and if it has an impact I'll try to figure something to avoid that without unsafe.
Not sure if you want to take on another dependency, but |
Ok, I tried but while it does work, there is still Windows which is out of scope for |
0a63e55
to
6626d76
Compare
I dropped the last commit after running the As I said, it is quite noisy and I am not sure what to make of it. (My hardware especially the disk are decidedly consumer-grade.) |
…t traffic in its Drop impl.
Ah, I found a way to at least avoid the option dance without unsafe code at the cost of an otherwise unnecessary reference increment-decrement-pair. Pushed that as a separate commit. |
Ok, I also reran the benchmark after changing it to run from a ramdisk to take my With that I am now reasonably confident that this does not introduce undue overhead and yields small but consistent improvements:
|
Merged. Thanks! |
The second commit removes any per-access overhead toWritablePage
but at the cost of some unsafe code relying on the uniqueness that was asserted once by callingArc::try_unwrap
before this change.