Require three epoch advancements for deallocation #416
Conversation
tomtomjhj
force-pushed
the
free-after-3
branch
from
0545c20 to
6200901
Compare
|
@jeehoonkang I will take a look. |
|
@tomtomjhj would you please rebase it on the current master? We recently fixed CI failures. |
tomtomjhj
force-pushed
the
free-after-3
branch
from
6200901 to
0c42306
Compare
| atomic::fence(Ordering::SeqCst); | ||
| } | ||
|
|
||
| // Now we validate that the value we read from the global epoch is not stale. |
There was a problem hiding this comment.
What is the motivation for validation phase? I'm afraid that this loop might slow things down a bit.
Could you run cargo bench and see if this affects performance?
There was a problem hiding this comment.
It's for helping the global epoch advancement in general. It doesn't seem to have a meaningful effect on the performance.
- master:
test multi_alloc_defer_free ... bench: 4,135,110 ns/iter (+/- 566,246)
test multi_defer ... bench: 2,154,671 ns/iter (+/- 212,984)
test single_alloc_defer_free ... bench: 43 ns/iter (+/- 0)
test single_defer ... bench: 16 ns/iter (+/- 0)
test multi_flush ... bench: 22,928,416 ns/iter (+/- 1,546,218)
test single_flush ... bench: 136 ns/iter (+/- 1)
test multi_pin ... bench: 4,179,789 ns/iter (+/- 167,156)
test single_pin ... bench: 5 ns/iter (+/- 0)
- free-after-3
test multi_alloc_defer_free ... bench: 4,176,331 ns/iter (+/- 513,890)
test multi_defer ... bench: 2,179,475 ns/iter (+/- 197,665)
test single_alloc_defer_free ... bench: 43 ns/iter (+/- 1)
test single_defer ... bench: 16 ns/iter (+/- 0)
test multi_flush ... bench: 21,997,370 ns/iter (+/- 3,973,497)
test single_flush ... bench: 135 ns/iter (+/- 0)
test multi_pin ... bench: 4,173,010 ns/iter (+/- 389,623)
test single_pin ... bench: 5 ns/iter (+/- 0)
- free-after-3 without validation
test multi_alloc_defer_free ... bench: 4,140,581 ns/iter (+/- 426,444)
test multi_defer ... bench: 2,160,064 ns/iter (+/- 196,742)
test single_alloc_defer_free ... bench: 43 ns/iter (+/- 0)
test single_defer ... bench: 16 ns/iter (+/- 0)
test multi_flush ... bench: 22,800,624 ns/iter (+/- 2,119,408)
test single_flush ... bench: 136 ns/iter (+/- 1)
test multi_pin ... bench: 4,228,682 ns/iter (+/- 380,561)
test single_pin ... bench: 5 ns/iter (+/- 0)
tomtomjhj
force-pushed
the
free-after-3
branch
from
0c42306 to
adfc043
Compare
|
bors r+ |
|
bors r- @tomtomjhj and I figured out that actually #238 is not fixed by this commit... More discussion will come by @tomtomjhj soon. |
Canceled |
|
Btw, I believe it's a good patch, though. I'm disapproving this PR to notify that it doesn't fix #238. |
tomtomjhj
changed the title
tomtomjhj
force-pushed
the
free-after-3
branch
from
adfc043 to
33f7baf
Compare
|
Status on this? |
|
@jeehoonkang Do you think this needs to be merged? Let's make a decision. |
ghost
mentioned this pull request
|
I'd like to merge it because this PR streamlines the correctness argument of crossbeam-epoch. bors r+ |
416: Require three epoch advancements for deallocation r=jeehoonkang a=tomtomjhj This patch implements the fix discussed in #238 (comment). * tag the Bag with the local epoch E * free at E+3 Co-authored-by: Jeehoon Kang <jeehoon.kang@sf.snu.ac.kr>
|
Build failed: |
|
CI test fails with an orthogonal error: https://github.com/crossbeam-rs/crossbeam/pull/416/checks?check_run_id=690982289#step:6:15 |
|
Let's merge manually. |
ghost
merged commit 
This patch implements the fix discussed in #238 (comment).