-
Notifications
You must be signed in to change notification settings - Fork 12
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
Simplify push_pop_{max, min} and replace_{max, min} implementations #8
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
==========================================
- Coverage 98.70% 98.55% -0.16%
==========================================
Files 3 3
Lines 541 552 +11
==========================================
+ Hits 534 544 +10
- Misses 7 8 +1
Continue to review full report at Codecov.
|
Hey, just saw this! Sorry for being slow. I have very limited time right now, but from a cursory glance, it looks excellent! |
This PR, along with #11, look awesome. Great work @calebsander ! |
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
==========================================
- Coverage 98.70% 98.55% -0.16%
==========================================
Files 3 3
Lines 541 552 +11
==========================================
+ Hits 534 544 +10
- Misses 7 8 +1
Continue to review full report at Codecov.
|
Also fixes a panic when dropping PeekMaxMut on a 1-element heap
Hopefully increases coverage
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
==========================================
+ Coverage 98.70% 99.15% +0.44%
==========================================
Files 3 3
Lines 541 589 +48
==========================================
+ Hits 534 584 +50
+ Misses 7 5 -2
Continue to review full report at Codecov.
|
peek_{max, min}_mut()
inpush_pop_{max, min}()
andreplace_{max, min}()
to reduce code duplication. Comparing the generated assembly, this seems to have a few performance effects (but I haven't benchmarked them):push_pop_{max, min}()
avoid swappingelement
to the heap if it is equal to max/minpush_pop_max()
andreplace_max()
each avoid 2 bounds checkspush_pop_min()
avoids a branch instructionreplace_{max, min}()
avoid bubbling up the inserted value if the heap is emptyreplace_max()
swaps the min withelement
in a register instead of swapping two indices in the heapPeek{Max, Min}Mut
struct is spilled to the stackPeek{Max, Min}Mut
if it was never mutably dereferenced (copied from Rust'sBinaryHeap
: Avoid useless sift_down when std::collections::binary_heap::PeekMut is never mutably dereferenced rust-lang/rust#75974).PeekMaxMut
on a 1-element heap (which thequickcheck
tests were now able to discover). Added a test for this.