Librafying of tools (cargo, rustdoc, fuzzer) #3835
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
lgtm. I ran this by the try bots today and they seemed happy. Unfortunately the windows build is broken, for unrelated reasons, and the win2 bot is down. This is the kind of thing I like the bots to be functional for. |
|
Somebody has finally started looking at our downed build machine. Hopefully it will be back up today. |
|
Now that the bots are all happy again, I'm running a rebased version of this on try. |
|
try passes on linux, mac, and freebsd. Hoping nothing is secretly broken on Windows (the try bot doesn't work on Windows, for unrelated reasons); merged to incoming. Thanks for your patience, @dbp ! |
bors
pushed a commit
to rust-lang-ci/rust
that referenced
this pull request
May 15, 2021
RalfJung
pushed a commit
to RalfJung/rust
that referenced
this pull request
Aug 30, 2024
…r=RalfJung Avoid extra copy by using `retain_mut` and moving the deletion into the closure Fixes the FIXME introduced in rust-lang#3833. Thanks to `@dmitrii-ubskii` for the idea 🙂
RalfJung
pushed a commit
to RalfJung/rust
that referenced
this pull request
Aug 30, 2024
Follow-up on rust-lang#3833 and rust-lang#3835. In these PRs, the TB GC was fixed to no longer cause a stack overflow. One test that motivated it was the test `fill::horizontal_line` in `tiny_skia`. But not causing stack overflows was not a large improvents, since it did not fix the fundamental issue: The tree was too large. The test now ran, but it required gigabytes of memory and hours of time, whereas it finishes within seconds in Stacked Borrows. The problem in that test was that it used [`slice::chunked`](https://doc.rust-lang.org/std/primitive.slice.html#method.chunks) to iterate a slice in chunks. That iterator is written to reborrow at each call to `next`, which creates a linear tree with a bunch of intermediary nodes, which also fragments the `RangeMap` for that allocation. The solution is to now compact the tree, so that these interior nodes are removed. Care is taken to not remove nodes that are protected, or that otherwise restrict their children.
RalfJung
pushed a commit
to RalfJung/rust
that referenced
this pull request
Aug 30, 2024
…e-gc, r=RalfJung Make Tree Borrows Provenance GC compact the tree Follow-up on rust-lang#3833 and rust-lang#3835. In these PRs, the TB GC was fixed to no longer cause a stack overflow. One test that motivated it was the test `fill::horizontal_line` in [`tiny-skia`](https://github.com/RazrFalcon/tiny-skia). But not causing stack overflows was not a large improvents, since it did not fix the fundamental issue: The tree was too large. The test now ran, but it required gigabytes of memory and hours of time (only for it to be OOM-killed 🤬), whereas it finishes within 24 seconds in Stacked Borrows. With this merged, it finishes in about 40 seconds under TB. The problem in that test was that it used [`slice::chunked`](https://doc.rust-lang.org/std/primitive.slice.html#method.chunks) to iterate a slice in chunks. That iterator is written to reborrow at each call to `next`, which creates a linear tree with a bunch of intermediary nodes, which also fragments the `RangeMap` for that allocation. The solution is to now compact the tree, so that these interior nodes are removed. Care is taken to not remove nodes that are protected, or that otherwise restrict their children. I am currently only 99% sure that this is sound, and I do also think that this could compact even more. So `@Vanille-N` please also have a look at whether I got the compacting logic right. For a more visual comparison, [here is a gist](https://gist.github.com/JoJoDeveloping/ae4a7f7c29335a4c233ef42d2f267b01) of what the tree looks like at one point during that test, with and without compacting. This new GC requires a different iteration order during accesses (since the current one can make the error messages non-deterministic), so it is rebased on top of rust-lang#3843 and requires that PR to be merged first.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a continuation of #3672 because github started thinking that I wanted to merge over 300 commits (all but 2 of which were already on incoming).
@catamorphism - this passes the tests that were failing before, and passes make check (on mac, 10.7).