proc_macro/bridge: remove client->server &HandleCounters passing.

[eddyb]

Just like #97461, this was inspired by @nnethercote's efforts in this area (see #97004 (comment)).
The purpose of the counters was never to protect from server-side misuse, only client-side misuse (e.g. a proc macro storing a proc_macro public API type in TLS), so the FIXME comment should suffice for now.

As this PR removes get_handle_counters (the only non-ZST Client field other than run, since #97461), it will allow a #[repr(transparent)] Client that only newtypes the run C ABI entry-point fn pointer _{(and in the context of e.g. wasm isolation, that's all you want, since you can reason about it from outside the wasm VM, as just a 32-bit "function table index", that you can pass to the wasm VM to call that function)}.

However, I haven't changed any #[repr] attributes in this PR to avoid unforseen perf interactions.

cc @mystor @bjorn3

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[eddyb]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit b251053 with merge d75ab565e15dc39a3d43092f34ca45733cf34a90...

[bors]

☔ The latest upstream changes (presumably #98186) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

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

[rust-timer]

Queued d75ab565e15dc39a3d43092f34ca45733cf34a90 with parent ff86b27, future comparison URL.

[mystor]

This looks good and should make future bridge mechanism changes a bit easier :-)

FWIW this does conflict with (one of the more sketchy) ideas from my (very old) PR #86816. Specifically the handling of non-blocking RPC for calls like TokenStream::clone() or stream concatenation was implemented in that PR by atomically generating a new handle to use for the response object on the client side, and sending it to the server to use for the result.

That approach was probably not the right one either way, and there are other ways to generate non-conflicting IDs on server and client if that turns out to be valuable in the future (e.g. count down starting at u32::MAX for client-generated IDs), but I figured it was worth noting :-)

[mystor]

Perhaps mention "the same dylib" or something like that? This could be interpreted as "the proc_macro_server::Rustc object should be the same for every invocation of a macro from each crate", which I don't think is what you're suggesting (as ideally I'd imagine that we we want to throw out HandleStore after each invocation of a proc macro, and only keep around the handle counter to avoid conflicts, which is done here by using a static for the counter).

[rust-timer]

Finished benchmarking commit (d75ab565e15dc39a3d43092f34ca45733cf34a90): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 🎉 relevant improvement found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.0%	-3.0%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.4%	4.4%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.8%	-2.8%	1
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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.

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

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²

[petrochenkov]

r? @mystor @nnethercote

[nnethercote]

I don't see any problems here, but I won't pretend to understand this code well enough to properly review changes, and it's clear that @mystor understands it better than I do.

Having said that:

• I am happy to see HandleCounters go away, much like in the 3rd commit of More proc macro tweaks #97445.
• The 2nd commit in More proc macro tweaks #97445 has the title "Move HandleStore into server.rs." and this description: "This fixes two FIXME comments, and makes things clearer, by not having server code in client.rs." Is that change reasonable? If so, perhaps it could be done as another commit in this PR. Or perhaps after this PR is merged it could be done in More proc macro tweaks #97445 after a rebase. I don't mind, whatever is quicker and easier for everybody.

[nnethercote]

Anyway, if @mystor is happy with this then I am happy to give r+.

[mystor]

LGTM. Only comment I have is the one I mentioned above (#98223 (comment)) about the misleading FIXME comment, but it's fine as-is.

[nnethercote]

@bors delegate=eddyb

Needs a rebase, which gives the opportunity to fix the comment as described by @mystor.

[bors]

✌️ @eddyb can now approve this pull request

[eddyb]

(FWIW there should be no need for a bors delegate, I can just r=... when it's ready)

Also, I've been meaning to reply to #98223 (comment) - I'd already forgotten how much that PR did (I only remembered the "moving HandleStore" part, sorry! makes more sense now why I've been thinking more about removing get_handle_counters lately, welp).
I want to go back and include those two commits (and give more credit in the PR description), while having my change be just the extra justification comment, but I won't get to it before the weekend.

[nnethercote]

(FWIW there should be no need for a bors delegate, I can just r=... when it's ready)

True, but I chose to use the official delegation mechanism because it's clearer than the alternatives.

[wesleywiser]

Hello @eddyb and @nnethercote! Just checking in on how this is coming along. From reading over the PR, it sounds like this (partially?) subsumes #97445 which we visited during this PR review in this week's compiler team triage meeting. If not, we're thinking of re-rolling reviewers on that PR as it seems to be straightforward cleanups.

[nnethercote]

This PR has r+ from me, it just needs to be rebased and a couple of comments added. Once that merges I will rebase #97445.

[JohnCSimon]

This PR has r+ from me, it just needs to be rebased and a couple of comments added. Once that merges I will rebase #97445.

Ping from triage:
@eddyb - Can you please resolve the merge conflicts?

[eddyb]

Can you please resolve the merge conflicts?

^{_{(the merge conflicts aren't the issue)}} When I said "the weekend" back in #98223 (comment) that turned out to be several weeks and I still haven't been able to catch up with a lot of rust-lang/rust things and other smaller stuff, sorry!

[apiraino]

friendly ping @eddyb when you have a a chance for the last touches (IIRC we're close to merge this 🙂 )

[JohnCSimon]

@eddyb
ping from triage - can you post your status on this PR? This PR has not received an update in a few months. Thank you!

[JohnCSimon]

Triage: I'm closing this due to inactivity

@rustbot label: +S-inactive