work towards removing headers from exchange allocations #7495
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
* stop using an atomic counter, this has a significant cost and valgrind will already catch these leaks * remove the extra layer of function calls * remove the assert of non-null in free, freeing null is well defined but throwing a failure from free will not be * stop initializing the `prev`/`next` pointers * abort on out-of-memory, failing won't necessarily work
|
memory bench: Saw ~740ms speedup in clock time, ~740ms speedup in cpuacct time. |
|
@cmr: well I don't expect there to be any noticeable change until the headers are actually gone. That's probably just from the atomic counter not being there, but that mostly has an effect in parallel code. |
|
This is working on Linux and Windows but not on OS X. I'm trying to figure out where it goes wrong. |
this is never read anymore
this is no longer used, exchange allocations do not set ref_count
the only user of the tydesc is ~fn, and it doesn't use this glue code
this makes the exchange allocation header completely unused, and leaves it uninitialized
bors
added a commit
that referenced
this pull request
Jun 30, 2013
With these changes, exchange allocator headers are never initialized, read or written to. Removing the header will now just involve updating the code in trans using an offset to only do it if the type contained is managed. The only thing blocking removing the initialization of the last field in the header was ~fn since it uses it to store the dynamic size/types due to captures. I temporarily switched it to a `closure_exchange_alloc` lang item (it uses the same `exchange_free`) and #7496 is filed about removing that. Since the `exchange_free` call is now inlined all over the codebase, I don't think we should have an assert for null. It doesn't currently ever happen, but it would be fine if we started generating code that did do it. The `exchange_free` function also had a comment declaring that it must not fail, but a regular assert would cause a failure. I also removed the atomic counter because valgrind can already find these leaks, and we have valgrind bots now. Note that exchange free does not currently print an error an out-of-memory when it aborts, because our `io` code may allocate. We could probably get away with a `#[rust_stack]` call to a `stdio` function but it would be better to make a write system call.
This was referenced Jul 3, 2013
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Jul 29, 2021
Improve conflicting rlibs error again changelog: none Now you can do `rm <paste>` and 🐇💨 ```text thread 'compile_test' panicked at ' ---------------------------------------------------------------------- ERROR: Found multiple rlibs for crates: `clippy_lints`, `clippy_utils` Try running `cargo clean` or remove the following files: target/debug/deps/libclippy_lints-9117c875159004e0.rlib \ target/debug/deps/libclippy_lints-fe45157be7ff9444.rlib \ target/debug/deps/libclippy_utils-5eba1e07a9846ed0.rlib \ target/debug/deps/libclippy_utils-ccbc08fcf64de262.rlib For details on this error see rust-lang/rust-clippy#7343 ---------------------------------------------------------------------- ```
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.
With these changes, exchange allocator headers are never initialized, read or written to. Removing the header will now just involve updating the code in trans using an offset to only do it if the type contained is managed.
The only thing blocking removing the initialization of the last field in the header was ~fn since it uses it to store the dynamic size/types due to captures. I temporarily switched it to a
closure_exchange_alloclang item (it uses the sameexchange_free) and #7496 is filed about removing that.Since the
exchange_freecall is now inlined all over the codebase, I don't think we should have an assert for null. It doesn't currently ever happen, but it would be fine if we started generating code that did do it. Theexchange_freefunction also had a comment declaring that it must not fail, but a regular assert would cause a failure. I also removed the atomic counter because valgrind can already find these leaks, and we have valgrind bots now.Note that exchange free does not currently print an error an out-of-memory when it aborts, because our
iocode may allocate. We could probably get away with a#[rust_stack]call to astdiofunction but it would be better to make a write system call.