Stop creating references unnecessarily to compare pointers by-address #452
Conversation
The test `common::deque::basics` contains several lines that look like this:
```
assert!(std::ptr::eq(head_d, unsafe { node1_ptr.as_ref() }));
```
What is happening is that we assert that `head_c`, a reference, is equal
by-address to `node1_ptr`, a `NonNull<_>`. However, the way we do this
is by creating a reference, instead of using `as_ptr`.
The current way has several downsides:
* It requires an `unsafe` block. Comparing pointer addresses is safe so
this should be unnecessary.
* **It is unsound**, at least under Tree Borrows rules.
The references created there violate the uniqueness of references/Boxes
created internally in the `Deque`, which trips the `Deque` later when
it tries to use these internally-held references.
* It is unnecessary -- comparing the result of `as_ptr` works just the same,
maybe even faster due to less intermediary methods.
As such, the test should be updated to only use `as_ptr`. When done, it
actually makes the tests pass under Tree Borrows.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
- Coverage 95.04% 94.97% -0.08%
==========================================
Files 44 44
Lines 20979 20979
==========================================
- Hits 19939 19924 -15
- Misses 1040 1055 +15 |
There was a problem hiding this comment.
Thank you for submitting this pull request with detailed explanations! That is very very helpful.
Actually, we are already running CI with miri on the deque and timer_wheel modules. However it did not detect the issue you reported as we only used stacked borrows model. I created another pull request #454 to use tree borrows in the CI in addition to stacked borrows.
|
Thanks for merging, and nice that you use Tree Borrows in your CI 🥳 But be warned that in miri, Tree Borrows currently comes with a performance impact, since there's a bit more to check. While I plan to optimize this further, I can't promise anything. Though if you notice any code that is significantly slower under TB than SB, reach out and I will see what I can do. Besides, can you also merge the sister PR for mini-moka? |
The test
common::deque::basicscontains several lines that look like this:What is happening is that we assert that
head_c, a reference, is equal by-address tonode1_ptr, aNonNull<_>. However, the way we do this is by creating a reference, instead of usingas_ptr.The current way has several downsides:
unsafeblock. Comparing pointer addresses is safe so this should be unnecessary.Deque, which trips theDequelater when it tries to use these internally-held references.as_ptrworks just the same, maybe even faster due to less intermediary methods.As such, the test should be updated to only use
as_ptr. When done, it actually makes the tests pass under Tree Borrows.See also moka-rs/mini-moka#31