[RFC] Remove RefCell from builder arena
#363
Conversation
marmeladema
force-pushed
the
arena-remove-refcell
branch
4 times, most recently
from
4270150
to
313d9b7
Compare
|
Wow, cool. Thanks! That Were you able to measure a performance improvement from this change? When I run the benchmarks in the repo, it looks like things slow down a bit. |
|
I'll check the benchmarks tomorrow, thank you for pointing me in the right direction! |
|
Yeah, the benchmarks in the repo just check the system clock before and after processing some randomized data. |
There were only two calls to `BuilderArenaImplInner::allocate_segment` and they were both followed by a call to `BuilderArenaImplInner::allocate`.
marmeladema
force-pushed
the
arena-remove-refcell
branch
from
313d9b7
to
66198dd
Compare
|
Not sure how reproducible my results are, but I tried running the benchmarks on Linux using the performance governor, with highest process priority on a mostly idle core. --- bench.master.log 2023-01-14 15:13:34.449673954 +0100
+++ bench.refcell.log 2023-01-14 15:10:17.616132232 +0100
@@ -1,71 +1,71 @@
running carsales with 10000 iterations
object none reuse
-0.274780196
+0.262815291
object none no-reuse
-0.314452609
+0.290588508
bytes none reuse
-0.323921303
+0.303009961
bytes none no-reuse
-0.367289562
+0.337183638
bytes packed reuse
-0.522327291
+0.483596268
bytes packed no-reuse
-0.583030896
+0.543108862
pipe none reuse
exit status: 0
-0.379240224
+0.359025021
pipe none no-reuse
exit status: 0
-0.415696975
+0.391269543
pipe packed reuse
exit status: 0
-0.591989011
+0.561077035
pipe packed no-reuse
exit status: 0
-0.657493158
+0.633822245
running catrank with 1000 iterations
object none no-reuse
-0.650825717
+0.630079401
bytes none no-reuse
-0.705408293
+0.674896325
bytes packed no-reuse
-0.943492149
+0.844120619
pipe none no-reuse
exit status: 0
-0.781824298
+0.73111535
pipe packed no-reuse
exit status: 0
-0.971964107
+0.939981865
running eval with 200000 iterations
object none no-reuse
-0.704358592
+0.633409412
bytes none no-reuse
-0.841810654
+0.786843015
bytes packed no-reuse
-1.291954872
+1.207431156
pipe none no-reuse
exit status: 0
-1.626859737
+1.523820484
pipe packed no-reuse
exit status: 0
-2.016225853
+1.960986856 |
| // have lifetime issues. (If `'a: 'b`, then a `&'a (BuilderArena + 'a)` can be | ||
| // converted to a `&'b (BuilderArena + 'b)`, but a `&'a mut (BuilderArena + 'a)` | ||
| // *cannot* be converted to a `&'b mut (BuilderArena + 'b)`. See some discussion here: | ||
| // https://botbot.me/mozilla/rust/2017-01-31/?msg=80228117&page=19 .) |
There was a problem hiding this comment.
Ah, so this comment captures the reason why I thought that the RefCell was necessary. (I updated it for dyn and reborrow here: 5ad7849.)
I wonder what changed so that this is now possible? Or maybe I was mistaken all along?
There was a problem hiding this comment.
Wow, this failed in Rust 1.62: https://godbolt.org/z/4j354zad3
and started working in Rust 1.63: https://godbolt.org/z/3hv5x3vrK
I'm a little bit worried that this means that the borrowchecker is now unsound! But if it's not, then this is good news for us, because it will let us make these methods take &mut self and therefore let us remove the RefCell.
There was a problem hiding this comment.
I think this is because NLL has been fully stabilized in 1.63.0: https://blog.rust-lang.org/2022/08/05/nll-by-default.html
There was a problem hiding this comment.
Looking at the code, I don't see any obvious reasons it should not compile or can be unsound.
There was a problem hiding this comment.
I can probably ask t-types team on zulip for a confirmation?
There was a problem hiding this comment.
Ah, and according to the referece (https://doc.rust-lang.org/reference/subtyping.html#variance),
dyn Trait<T> + 'a is "covariant" in 'a. Pre-NLL it was "invariant", if I understand correctly.
There was a problem hiding this comment.
Actually, I can get my little example to typecheck on an even older rustc (e.g. 1.30) if I avoid the ..*self syntax:
https://godbolt.org/z/8EGx4v3zK
There was a problem hiding this comment.
There was a problem hiding this comment.
If I understand correctly, 1.63.0 just made it work with the ..*self syntax, but it was working before using regular field assignment. So arguably, we could have introduced such code before 1.63.0 and get rid of RefCell already.
So answering your first question from zulip:
Can I rely on this continuing to typecheck? I'd like to migrate a bunch of my code to depend on this, because I had been working around it previously.
Unless there is something inherently unsound about accepting this pattern; I fail to see why the compiler would suddenly reject it. That code has been working for years and such pattern is probably used in various places; so rejecting it would be tricky.
Given all of this, do you see any blocker that would prevent merging this?
There was a problem hiding this comment.
I want to convince myself it's not unsound first. :)
I think this PR should be considered a breaking change and that we should do a version bump soon including it and some of the other stuff that's been waiting on a version bump.
|
Superseded by #365. |
Based on #362 so only the last commit is important here.
The usage of
RefCellwas noticeable in production flamegraphs I gathered and since it is not strictly required, this proposal attempts to remove it.There are probably other ways but this way was pretty straightforward, even though, very invasive.