Skip to content

Reduce TypedArena creations in check_match. #71975

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged

Conversation

nnethercote
Copy link
Contributor

check_match creates a new TypedArena for every call to
create_and_enter. DHAT tells me that each TypedArena typically is
barely used, with typically a single allocation per arena.

This commit moves the TypedArena creation outwards a bit, into
check_match, and then passes it into create_and_enter. This reduces
the number of arenas created by about 4-5x, for a very small perf win.
(Moving the arena creation further outwards is hard because
check_match is a query.)

r? @oli-obk

`check_match` creates a new `TypedArena` for every call to
`create_and_enter`. DHAT tells me that each `TypedArena` typically is
barely used, with typically a single allocation per arena.

This commit moves the `TypedArena` creation outwards a bit, into
`check_match`, and then passes it into `create_and_enter`. This reduces
the number of arenas created by about 4-5x, for a very small perf win.
(Moving the arena creation further outwards is hard because
`check_match` is a query.)
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2020
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented May 7, 2020

⌛ Trying commit cbc577f with merge a303191d82d1d452f5dfddc9efd98c3341e64b66...

@bors
Copy link
Collaborator

bors commented May 7, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: a303191d82d1d452f5dfddc9efd98c3341e64b66 (a303191d82d1d452f5dfddc9efd98c3341e64b66)

@rust-timer
Copy link
Collaborator

Queued a303191d82d1d452f5dfddc9efd98c3341e64b66 with parent 97f3eee, future comparison URL.

@oli-obk
Copy link
Contributor

oli-obk commented May 7, 2020

Why are we even using an arena here? We use Box<PatKind<'tcx>> for cycles (and PatKind just uses Pat by value, not interned). Is this a leftover from previous refactorings or was the plan to use interned pats in PatKind and stop boxin PatKind?

cc @varkor do you remember anything here?

@varkor
Copy link
Member

varkor commented May 7, 2020

I don't remember why TypedArena is used here. cc @Nadrieril who probably has this code more in-cache.

@Nadrieril
Copy link
Member

Nadrieril commented May 7, 2020

It's used a lot in _match, where the matrix stores only arena-allocated Pats. Since the matrix gets cloned a lot, I imagine this is good for performance, as opposed to owned Pats ?

It has a single call site.
@nnethercote
Copy link
Contributor Author

nnethercote commented May 7, 2020

It's used a lot in _match, where the matrix stores only arena-allocated Pats. Since the matrix gets cloned a lot, I imagine this is good for performance, as opposed to owned Pats ?

I tried removing the arena and although I never got it close to compiling (so many lifetime changes needed) I concluded that this was probably the reason.

@nnethercote nnethercote force-pushed the reduce-TypedArena-creations-in-check_match branch from 1a59216 to d26d187 Compare May 7, 2020 22:58
@oli-obk
Copy link
Contributor

oli-obk commented May 8, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented May 8, 2020

📌 Commit d26d187 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#71581 (Unify lints handling in rustdoc)
 - rust-lang#71710 (Test for zero-sized function items not ICEing)
 - rust-lang#71970 (Improve bitcode generation for Apple platforms)
 - rust-lang#71975 (Reduce `TypedArena` creations in `check_match`.)
 - rust-lang#72003 (allow wasm target for rustc-ap-rustc_span)
 - rust-lang#72017 (Work around ICEs during cross-compilation for target, ast, & attr)

Failed merges:

r? @ghost
@bors bors merged commit 0c8ef47 into rust-lang:master May 8, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit a303191d82d1d452f5dfddc9efd98c3341e64b66, comparison URL.

@nnethercote nnethercote deleted the reduce-TypedArena-creations-in-check_match branch May 10, 2020 22:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants