[parallel-queries] Make CrateMetadata::cnum_map immutable #50502
Labels
A-parallel-queries
Area: Parallel query execution
C-enhancement
Category: An issue proposing an enhancement or a PR with one.
E-mentor
Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
WG-compiler-performance
Working group: Compiler Performance
Comments
michaelwoerister
added
E-easy
|
It seems that |
|
@rleungx Oh, you are right of course. In that case, I'll remove the |
michaelwoerister
removed
the
E-easy
|
#50532 implements a partial solution. We still have a lock, but its around data that is accessed a lot less. |
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this issue
May 10, 2018
… r=Zoxc Don't use Lock for heavily accessed CrateMetadata::cnum_map. The `cnum_map` in `CrateMetadata` is used for two things: 1. to map `CrateNums` between crates (used a lot during decoding) 2. to construct the (reverse) post order of the crate graph For the second case, we need to modify the map after the fact, which is why the map is wrapped in a `Lock`. This is bad for the first case, which does not need the modification and does lots of small reads from the map. This PR splits case (2) out into a separate `dependencies` field. This allows to make the `cnum_map` immutable (and shifts the interior mutability to a less busy data structure). Fixes rust-lang#50502 r? @Zoxc
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this issue
May 10, 2018
… r=Zoxc Don't use Lock for heavily accessed CrateMetadata::cnum_map. The `cnum_map` in `CrateMetadata` is used for two things: 1. to map `CrateNums` between crates (used a lot during decoding) 2. to construct the (reverse) post order of the crate graph For the second case, we need to modify the map after the fact, which is why the map is wrapped in a `Lock`. This is bad for the first case, which does not need the modification and does lots of small reads from the map. This PR splits case (2) out into a separate `dependencies` field. This allows to make the `cnum_map` immutable (and shifts the interior mutability to a less busy data structure). Fixes rust-lang#50502 r? @Zoxc
|
Is this still a partial solution?
…On Thu, May 10, 2018, 10:04 PM bors ***@***.***> wrote:
Closed #50502 <#50502> via #50532
<#50532>.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#50502 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAyu2OScGgVW0_UzuzJmTDaTQ2CSNRt4ks5txPGkgaJpZM4T1EXS>
.
|
|
It's partial (because it does not reduce the amount of shared mutable state) but it addresses my main concern by not requiring to keep the Removing more mutable state from |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The CrateMetadata::cnum_map field is wrapped in a
Lockbut it does not seem to be mutated anywhere. Less mutable state is always better. To solve this task, remove theLockand make things compile again.cc @rust-lang/wg-compiler-performance
The text was updated successfully, but these errors were encountered: