proc_macro/bridge: use the cross-thread executor for nested proc-macros #101414

mystor · 2022-09-04T17:15:20Z

While working on some other changes in the bridge, I noticed that when
running a nested proc-macro (which is currently only possible using
the unstable TokenStream::expand_expr), any symbols held by the
proc-macro client would be invalidated, as the same thread would be used
for the nested macro by default, and the interner doesn't handle nested
use.

After discussing with @eddyb, we decided the best approach might be to
force the use of the cross-thread executor for nested invocations, as it
will never re-use thread-local storage, avoiding the issue. This
shouldn't impact performance, as expand_expr is still unstable, and
infrequently used.

This was chosen rather than making the client symbol interner handle
nested invocations, as that would require replacing the internal
interner Vec with a BTreeMap (as valid symbol id ranges could now be
disjoint), and the symbol interner is known to be fairly perf-sensitive.

This patch adds checks to the execution strategy to use the cross-thread
executor when doing nested invocations. An alternative implementation
strategy could be to track this information in the ExtCtxt, however a
thread-local in the proc_macro crate was chosen to add an assertion so
that rust-analyzer is aware of the issue if it implements
expand_expr in the future.

r? @eddyb

rustbot · 2022-09-04T17:15:23Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred in library/proc_macro/src/bridge

cc @rust-lang/wg-rls-2

eddyb

r=me with the nits fixed but make sure to also note this on the expand_expr tracking issue, as something to consider when stabilizing (assuming you don't want to block on a wider discussion now)

library/proc_macro/src/bridge/server.rs

@eddyb

While working on some other changes in the bridge, I noticed that when running a nested proc-macro (which is currently only possible using the unstable `TokenStream::expand_expr`), any symbols held by the proc-macro client would be invalidated, as the same thread would be used for the nested macro by default, and the interner doesn't handle nested use. After discussing with @eddyb, we decided the best approach might be to force the use of the cross-thread executor for nested invocations, as it will never re-use thread-local storage, avoiding the issue. This shouldn't impact performance, as expand_expr is still unstable, and infrequently used. This was chosen rather than making the client symbol interner handle nested invocations, as that would require replacing the internal interner `Vec` with a `BTreeMap` (as valid symbol id ranges could now be disjoint), and the symbol interner is known to be fairly perf-sensitive. This patch adds checks to the execution strategy to use the cross-thread executor when doing nested invocations. An alternative implementation strategy could be to track this information in the `ExtCtxt`, however a thread-local in the `proc_macro` crate was chosen to add an assertion so that `rust-analyzer` is aware of the issue if it implements `expand_expr` in the future. r? @eddyb

eddyb · 2022-09-04T18:51:34Z

@bors r+ rollup=never (new state in potentially critical path, hopefully perf is neutral 🤞)

bors · 2022-09-04T18:51:36Z

📌 Commit efda497 has been approved by eddyb

It is now in the queue for this repository.

bors · 2022-09-05T07:07:41Z

⌛ Testing commit efda497 with merge 5b4bd15...

bors · 2022-09-05T09:48:43Z

☀️ Test successful - checks-actions
Approved by: eddyb
Pushing 5b4bd15 to master...

rust-timer · 2022-09-05T16:50:48Z

Finished benchmarking commit (5b4bd15): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.9%	[0.4%, 1.4%]	10
Regressions ❌ (secondary)	1.1%	[0.6%, 1.7%]	9
Improvements ✅ (primary)	-1.2%	[-1.3%, -1.2%]	2
Improvements ✅ (secondary)	-3.4%	[-3.9%, -3.0%]	6
All ❌✅ (primary)	0.5%	[-1.3%, 1.4%]	12

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
Improvements ✅ (primary)	-1.5%	[-2.2%, -0.9%]	3
Improvements ✅ (secondary)	-2.3%	[-3.6%, -1.7%]	5
All ❌✅ (primary)	-1.5%	[-2.2%, -0.9%]	3

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)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

eddyb · 2022-09-05T18:13:06Z

library/proc_macro/src/bridge/server.rs

+impl RunningSameThreadGuard {
+    fn new() -> Self {
+        let already_running = ALREADY_RUNNING_SAME_THREAD.replace(true);
+        assert!(
+            !already_running,
+            "same-thread nesting (\"reentrance\") of proc macro executions is not supported"
+        );
+        RunningSameThreadGuard(())
+    }
+}
+
+impl Drop for RunningSameThreadGuard {
+    fn drop(&mut self) {
+        ALREADY_RUNNING_SAME_THREAD.set(false);
+    }
+}


Oops, these two functions may need #[inline]? I should've really ran perf, my bad :(

@eddyb

This is a potential fix to the perf regression from rust-lang#101414. r? @eddyb

pnkfelix · 2022-09-14T04:00:03Z

As noted elsewhere #101461 (comment) ...

... the regressions flagged here are almost certainly noise from recently arising bimodality in certain benchmarks.

@rustbot label: perf-regression-triaged

rust-highfive assigned eddyb Sep 4, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 4, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2022

eddyb suggested changes Sep 4, 2022

View reviewed changes

library/proc_macro/src/bridge/server.rs Outdated Show resolved Hide resolved

library/proc_macro/src/bridge/server.rs Outdated Show resolved Hide resolved

library/proc_macro/src/bridge/server.rs Outdated Show resolved Hide resolved

mystor force-pushed the pm_nested_cross_thread branch from dc28e1d to d5b5c91 Compare September 4, 2022 18:02

This comment has been minimized.

Sign in to view

mystor force-pushed the pm_nested_cross_thread branch from d5b5c91 to efda497 Compare September 4, 2022 18:06

eddyb approved these changes Sep 4, 2022

View reviewed changes

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 Sep 4, 2022

mystor mentioned this pull request Sep 4, 2022

Tracking Issue for proc_macro_expand #90765

Open

5 tasks

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2022

bors merged commit 5b4bd15 into rust-lang:master Sep 5, 2022

rustbot added this to the 1.65.0 milestone Sep 5, 2022

rustbot added the perf-regression Performance regression. label Sep 5, 2022

eddyb reviewed Sep 5, 2022

View reviewed changes

mystor added a commit to mystor/rust that referenced this pull request Sep 5, 2022

proc_macro/bridge: Add #[inline] to RunningSameThreadGuard methods

4913884

This is a potential fix to the perf regression from rust-lang#101414. r? @eddyb

mystor mentioned this pull request Sep 5, 2022

proc_macro/bridge: Add #[inline] to RunningSameThreadGuard methods #101461

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proc_macro/bridge: use the cross-thread executor for nested proc-macros #101414

proc_macro/bridge: use the cross-thread executor for nested proc-macros #101414

mystor commented Sep 4, 2022

rustbot commented Sep 4, 2022

eddyb left a comment •

edited

Loading

This comment has been minimized.

eddyb commented Sep 4, 2022

bors commented Sep 4, 2022

bors commented Sep 5, 2022

bors commented Sep 5, 2022

rust-timer commented Sep 5, 2022

eddyb Sep 5, 2022

pnkfelix commented Sep 14, 2022

proc_macro/bridge: use the cross-thread executor for nested proc-macros #101414

proc_macro/bridge: use the cross-thread executor for nested proc-macros #101414

Conversation

mystor commented Sep 4, 2022

rustbot commented Sep 4, 2022

eddyb left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

eddyb commented Sep 4, 2022

bors commented Sep 4, 2022

bors commented Sep 5, 2022

bors commented Sep 5, 2022

rust-timer commented Sep 5, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

eddyb Sep 5, 2022

Choose a reason for hiding this comment

pnkfelix commented Sep 14, 2022

eddyb left a comment •

edited

Loading