Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

refactor WorkerLocal for parallel compiler #109478

Closed
wants to merge 1 commit into from

Conversation

SparrowLii
Copy link
Member

part of #101566
This PR refactor WorkerLocal for parallel compiler, facilitating code review and perf test.

ps. refactored WorkerLocal is not Send or Sync. It depends on #107586 to get thread safety.

@rustbot
Copy link
Collaborator

rustbot commented Mar 22, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 22, 2023
@SparrowLii SparrowLii changed the title refactor WorkerLocal refactor WorkerLocal for parallel compiler Mar 22, 2023
@SparrowLii
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 22, 2023
@bors
Copy link
Contributor

bors commented Mar 22, 2023

⌛ Trying commit 477e410 with merge fe80553693db52675c166e07a4e745050855ba41...

// Safety: `inner` would never be accessed when multiple threads
WorkerLocal {
single_thread: false,
inner: unsafe { MaybeUninit::uninit().assume_init() },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems dangerous...
I am not entirely sure if the semantics of this have been fixed yet, but reading https://github.com/rust-lang/unsafe-code-guidelines/blob/master/active_discussion/validity.md it seems like this would be UB under that (since the assignment does a typed copy at type T, which does not allow uninit).
Either way, this is certainly a pattern that is discouraged, and it seems like the compiler should set an example here...
It seems much better to use a union between inner and mt_inner here, since it is guaranteed to only access the right field. (Or even better, an enum, since single_thread then functions as a discriminant... which basically makes it a homegrown enum anyways)

Copy link
Contributor

@RossSmyth RossSmyth Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is very unsound because rustc emits noundef for almost all types. So this is immediately UB from LLVM's PoV, and the current thought for Rust rules (with no real thoughts on it not being UB in the future).

Copy link
Member Author

@SparrowLii SparrowLii Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense. I am a little worried about the efficiency of using enum or union, maybe it is better to use inner: Option<T> here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what the efficiency problem of an enum or union would be - checking the discriminant of an enum should be basically the same as checking if self.single_thread {, right?
(Additionally, using an Option would add increase the size by adding the Options discriminant in addition to single_thread)
I have opened #109528 to test the performance of my suggestion.

Copy link
Member Author

@SparrowLii SparrowLii Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using enum will prevent LLVM from making the best optimizations in many cases. For example, the perf result of this commit: #101566 (comment)

And the use of union will cause the compiler to add a lot of stuffs that trigger unwind due to union access errors, which will also reduce the optimization effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But Option is also an enum, so it should have the same effect?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use unwrap which is a const function when signle_thread so i guess it is relatively more efficient

@bors
Copy link
Contributor

bors commented Mar 22, 2023

☀️ Try build successful - checks-actions
Build commit: fe80553693db52675c166e07a4e745050855ba41 (fe80553693db52675c166e07a4e745050855ba41)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fe80553693db52675c166e07a4e745050855ba41): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.3%, 1.0%] 6
Regressions ❌
(secondary)
0.5% [0.3%, 0.8%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.7% [-4.6%, -0.3%] 14
All ❌✅ (primary) 0.6% [0.3%, 1.0%] 6

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.6% [0.4%, 0.7%] 5
Improvements ✅
(primary)
-1.5% [-2.2%, -0.9%] 2
Improvements ✅
(secondary)
-1.1% [-1.1%, -1.1%] 1
All ❌✅ (primary) -1.5% [-2.2%, -0.9%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.9% [0.5%, 1.2%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.8% [-0.8%, -0.8%] 1
All ❌✅ (primary) - - 0

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 22, 2023
@Zoxc
Copy link
Contributor

Zoxc commented Mar 25, 2023

Keep in mind that this needs to be an improvement with #107782 applied.

@SparrowLii
Copy link
Member Author

I think we will finish this work this month

@SparrowLii
Copy link
Member Author

SparrowLii commented Nov 25, 2023

close this as it's already landed

#117435

@SparrowLii SparrowLii closed this Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants