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

Proc macro tweaks #97004

Merged
merged 12 commits into from
May 27, 2022
Merged

Proc macro tweaks #97004

merged 12 commits into from
May 27, 2022

Conversation

nnethercote
Copy link
Contributor

Various improvements I spotted while looking through the proc macro code.

r? @eddyb

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 13, 2022
@rust-highfive
Copy link
Collaborator

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 r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. 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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2022
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

@bjorn3
Copy link
Member

bjorn3 commented May 13, 2022

Apart from the two comments I left everything looks fine.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Same as @bjorn3, primary concern is Closure<'a needing its lifetime, and there's a typo in a comment.

library/proc_macro/src/bridge/server.rs Outdated Show resolved Hide resolved
Comment on lines -9 to +10
pub struct Buffer<T: Copy> {
data: *mut T,
pub struct Buffer {
data: *mut u8,
Copy link
Member

Choose a reason for hiding this comment

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

For the record, I think the reason I made this generic was to avoid accidentally doing the wrong thing because the types happened to match up (especially since there's unsafe code here) but I'll review this carefully and hopefully this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would any type other than u8 make sense here? Doesn't seem like it, especially given that Buffer<u8> was hardcoded in a bunch of places anyway.

Copy link
Member

Choose a reason for hiding this comment

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

That's not what I meant, but rather that writing parametric code can sometimes help avoid making mistakes (which can be catastrophic in unsafe code).

It doesn't make sense at all from the perspective of whether e.g. a C++ container might be templated or not, but in does in Rust because of the type-checking in the generic form.

Anyway it doesn't matter much, I was just explaining that Foo<T> can still make sense even if it's only ever used as a single Foo<Concrete> and the generic nature "isn't taken advantage of".

Hopefully we can remove this Buffer abstraction and move more to e.g. a model closer to read/write syscalls (or io::{Read,Write} traits I suppose, at a higher level).

library/proc_macro/src/bridge/client.rs Outdated Show resolved Hide resolved
library/proc_macro/src/bridge/closure.rs Outdated Show resolved Hide resolved
// to avoid borrow conflicts from borrows started by `&mut` arguments.
// to match the ordering in `reverse_decode`.
Copy link
Member

Choose a reason for hiding this comment

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

I wrote a big comment I just had to remove, heh, until I figured out what was meant by this change.

So yeah it is about the borrow-checker but only directly so for reverse_decode and then reverse_encode has to play along.

I would still maybe add "(which is forced by borrowck)" or something at the end of the comment just to not make it look like the order is arbitrary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"to match the ordering in reverse_decode" was precisely my attempt to make it clear that the order is not arbitrary. I'm happy to hear alternative wording suggestions.

library/proc_macro/src/bridge/handle.rs Outdated Show resolved Hide resolved
@nnethercote nnethercote force-pushed the proc-macro-tweaks branch 3 times, most recently from 6166758 to 4e7fe1a Compare May 16, 2022 02:17
@nnethercote
Copy link
Contributor Author

I have addressed the comments, and added a small new commit "Rename ProcMacroDerive as DeriveProcMacro."

@nnethercote
Copy link
Contributor Author

I added another commit: "Inline and remove Bridge::enter."

@nnethercote
Copy link
Contributor Author

I have added a few more commits, including "Move HandleStore into server.rs" which fixes a couple of FIXME(eddyb) comments.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

LGTM, modulo some comments, and excluding the "inline Bridge::enter" commit - having a really hard time reviewing it with the s/b/buf change thrown in, can that be its commit?

Comment on lines 8 to 21
/// Declare an associated item of one of the traits below, optionally
/// adjusting it (i.e., adding bounds to types and default bodies to methods).
macro_rules! associated_item {
(type FreeFunctions) => (type FreeFunctions: 'static;);
(type TokenStream) => (type TokenStream: 'static + Clone;);
(type TokenStreamBuilder) => (type TokenStreamBuilder: 'static;);
(type TokenStreamIter) => (type TokenStreamIter: 'static + Clone;);
(type Group) => (type Group: 'static + Clone;);
(type Punct) => (type Punct: 'static + Copy + Eq + Hash;);
(type Ident) => (type Ident: 'static + Copy + Eq + Hash;);
(type Literal) => (type Literal: 'static + Clone;);
(type SourceFile) => (type SourceFile: 'static + Clone;);
(type MultiSpan) => (type MultiSpan: 'static;);
(type Diagnostic) => (type Diagnostic: 'static;);
(type Span) => (type Span: 'static + Copy + Eq + Hash;);
pub trait Types {
type FreeFunctions: 'static;
type TokenStream: 'static + Clone;
type TokenStreamBuilder: 'static;
type TokenStreamIter: 'static + Clone;
type Group: 'static + Clone;
type Punct: 'static + Copy + Eq + Hash;
type Ident: 'static + Copy + Eq + Hash;
type Literal: 'static + Clone;
type SourceFile: 'static + Clone;
type MultiSpan: 'static;
type Diagnostic: 'static;
type Span: 'static + Copy + Eq + Hash;
}
Copy link
Member

Choose a reason for hiding this comment

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

It would really be nice if this info was in with_api! instead, it just sucks that it would have to look like : (...); instead of : ...; to be easily digestible by proc macros.

Copy link
Contributor Author

@nnethercote nnethercote May 18, 2022

Choose a reason for hiding this comment

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

I thought about adding these bounds to with_api_types! so that this trait could be a with_api_types invocations and decided against it.

I understand the appeal of DRY and the platonic goal of encoding all of the API information in a single macro so that API changes (e.g. adding new types and/or methods) only requires modifying a single code location. But I don't think that's so important. If adding a new type and/or method requires modifying two or three places (which the compiler will point out for you) it's not that bad.

What is bad, and I have had to struggle with for a couple of days now, is how hard it is to read really macro-heavy code. I have found this bridge code hard to read. I find it hard to do macro substitution in my head. I've written out parts of the expansions of several macro invocations in a separate text file just to see what they look like. I'm contemplating putting such example expansions into comments to help future readers of the code (including me). And this was a case where macro invocation would have saved very little in terms of code length.

Comment on lines 178 to 200
// Similar to `with_api`, but only lists the types, and they are divided into
// the two storage categories.
macro_rules! with_api_types {
($m:ident) => {
$m! {
'owned:
FreeFunctions,
TokenStream,
TokenStreamBuilder,
TokenStreamIter,
Group,
Literal,
SourceFile,
MultiSpan,
Diagnostic,

'interned:
Punct,
Ident,
Span,
}
};
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't great and it would be useful to have this information either in with_api! entirely or not at all there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a clear improvement over the existing code. E.g. this macro is right next to with_api!, previously this information was in the define_handles! macro in a different file.

Comment on lines 14 to 16
pub struct HandleCounters {
$($oty: AtomicUsize,)*
$($ity: AtomicUsize,)*
pub(super) struct HandleCounters {
$(pub(super) $oty: AtomicUsize,)*
$(pub(super) $ity: AtomicUsize,)*
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the fields being public is acceptable. The FIXME you went for is likely tied to the fact that it's impossible to have HandleStore::new defined anywhere other than in this module.

You could maybe move everything except HandleStore::new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is it not acceptable? The new division seems better: all the HandleStore stuff is in server.rs, where it belongs. HandleStore does need access to the fields of HandleCounters, but that makes sense given that HandleStore is layered on top of HandleCounters.

(Why is HandleCounters on the client side, BTW? Is it so that each proc macro gets its own separate set of counters? Would there be a problem if it was on the server side and all proc macros shared one set of counters?)

Some good news: your comment also made me realize that there was room for improvement in that last commit. I have removed the pub(super) markers on HandleStore, HandleStore's fields, HandleStore::new, MarkedTypes. I have also removed all the server:: qualifiers from the code moved into server.rs. Thanks for that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is HandleCounters on the client side, BTW?

I just tried moving it into server.rs and everything worked fine. All tests pass, and it removed the need for the pub(super) markings.

Maybe there's a reason for having it in client.rs, but there's no apparent explanation in either the code or the test suite. So I've pushed the commit to this PR in case it's usable.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Can you split these commits off into a separate PR?

  • Inline and remove Bridge::enter.
  • Move HandleStore into server.rs.
  • Move counters into server.rs.

(r=me on the 12 other commits at the time of this writing, ideally this will land even if I forget about it for another week)

Everything else is small and largely uncontroversial tweaks, and I think this PR has grown past its original scope, and it's hard for me to reason directly about it (e.g. there's FIXMEs I left in the code that seem to lack the precise reason I couldn't do that specific change originally).

@eddyb
Copy link
Member

eddyb commented May 27, 2022

Why is HandleCounters on the client side, BTW?

I just tried moving it into server.rs and everything worked fine. All tests pass, and it removed the need for the pub(super) markings.

Maybe there's a reason for having it in client.rs, but there's no apparent explanation in either the code or the test suite. So I've pushed the commit to this PR in case it's usable.

Replying to this (#97004 (comment)) here so that it doesn't get lost in review comments.

Just to be clear, the code compiling and the tests passing doesn't mean much about failure modes - we generally (at most) have tests for how the proc macro API is meant to be used, and maybe some high-level errors like trying to create invalid tokens, not low-level abuse (which can be tricky to test, or at the very least require a lot of scaffolding and the test itself may become brittle), or even internal properties of the code that don't affect its behavior (not really testable).

(For example, you can't break compilation or tests by making implementation details of unsafe code pub, but it is unsound because then someone can come in, break your abstractions, and cause UB)


The reason for client-side HandleCounters is to stop servers from reusing handles the client may already have (and has snuck into TLS) - that is, we want to always error when using a handle after the invocation "session" that allocated it ended (or after it's dropped, if an owned handle), no matter what safe code does (in the proc macro the user wrote - with wasm isolation we could theoretically even guarantee this for unsafe code as well).

(Apologies for this sort of scenario not being described in code comments, it took a long time to get that code landed so it's possible near the end I didn't even remember myself in great detail every decision)


There is, however, a simplification that can be made: the server can account for never reusing handles if it's the only server that is talking to that "in-memory" instance of the proc macro dylib.

This would always be true for process/wasm isolation (where the get_handle_counters model would be annoying/inefficient to support), because the server would be in charge of "running" the client, to an extent that would guarantee unique control.

Even for the in-process dlopen'd proc macros of today this is case in practice (since the dlopener tends to have unique control), but unlike IPC or wasm FFI there's no guarantee of there being an unique "other side" (like an IPC pipe/socket/etc. or the wasm VM).

So maybe all we need is the client to remember the fn pointer in the dispatch callback (even without #97045, this should be possible with a -> ptr::NonNull<()> method on Closure), in e.g. an AtomicPtr and panic (inside the catch_unwind that will serialize an Err back to the server) if two different servers are observed by the same client.

This has the nice added benefit of removing one of the blockers for any kind of proc macro isolation that doesn't share memory between the client and the server, but I don't think it should be done in this PR (feel free to reference this comment or quote parts of it wherever such an endeavor ends up).

cc @mystor

`u8` is the only type that makes sense for `T`, as demonstrated by the
fact that several impls and functions are hardwired to `Buffer<u8>`.
This gives the more obvious derive/attr/bang distinction, and reduces
code size slightly.
Similar to the existing `AttrProcMacro` trait.
So it matches the existing `AttrProcMacro` and `BangProcMacro` types.
`reverse_encode` isn't necessary to please the borrow checker, it's to
match the ordering done by `reverse_decode`.
There is some non-obvious information required to understand them.
@nnethercote
Copy link
Contributor Author

#97445 is the follow-up.

@bjorn3
Copy link
Member

bjorn3 commented May 27, 2022

and panic (inside the catch_unwind that will serialize an Err back to the server) if two different servers are observed by the same client.

That will break running multiple rustc sessions in the same process. The proc macro will keep being loaded, but rustc will forget all state (or at least it should).

@bors
Copy link
Contributor

bors commented May 27, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2022
@bors bors merged commit f558990 into rust-lang:master May 27, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 27, 2022
@nnethercote nnethercote deleted the proc-macro-tweaks branch May 27, 2022 09:37
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f558990): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
0.2% 0.2% 1
Regressions 😿
(secondary)
0.6% 1.5% 10
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 0.2% 0.2% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
1.7% 2.7% 3
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
2.6% 2.7% 2
Regressions 😿
(secondary)
3.6% 5.4% 9
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.6% 2.7% 2

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

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

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the perf-regression Performance regression. label May 27, 2022
@nnethercote
Copy link
Contributor Author

Surprising perf regressions. I'll take a look on Monday.

@eddyb
Copy link
Member

eddyb commented May 27, 2022

and panic (inside the catch_unwind that will serialize an Err back to the server) if two different servers are observed by the same client.

That will break running multiple rustc sessions in the same process. The proc macro will keep being loaded, but rustc will forget all state (or at least it should).

@bjorn3 Just as the proc macro will remain loaded, so will librustc_driver.so and in that process it would always have the same address for the callback function pointer in the Bridge - worst case, we can also send the address of a server-side static tied to proc_macro::bridge::server, so that it wouldn't even change if rustc is monomorphizing proc_macro::bridge::server functionality multiple times (we also should have some basic "version" negotiation to limit accidents but I'm not sure how to compute one short of adding a build.rs to library/proc_macro that hashes all files in its src...).

bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2022
proc_macro: don't pass a client-side function pointer through the server.

Before this PR, `proc_macro::bridge::Client<F>` contained both:
* the C ABI entry-point `run`, that the server can call to start the client
* some "payload" `f: F` passed to that entry-point
  * in practice, this was always a (client-side Rust ABI) `fn` pointer to the actual function the proc macro author wrote, i.e. `#[proc_macro] fn foo(input: TokenStream) -> TokenStream`

In other words, the client was passing one of its (Rust) `fn` pointers to the server, which was passing it back to the client, for the client to call (see later below for why that was ever needed).

I was inspired by `@nnethercote's` attempt to remove the `get_handle_counters` field from `Client` (see rust-lang#97004 (comment)), which combined with removing the `f` ("payload") field, could theoretically allow for a `#[repr(transparent)]` `Client` that mostly just newtypes the C ABI entry-point `fn` pointer <sub>(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)</sub>.

<hr/>

So this PR removes that "payload". But it's not a simple refactor: the reason the field existed in the first place is because monomorphizing over a function type doesn't let you call the function without having a value of that type, because function types don't implement anything like `Default`, i.e.:
```rust
extern "C" fn ffi_wrapper<A, R, F: Fn(A) -> R>(arg: A) -> R {
    let f: F = ???; // no way to get a value of `F`
    f(arg)
}
```
That could be solved with something like this, if it was allowed:
```rust
extern "C" fn ffi_wrapper<
    A, R,
    F: Fn(A) -> R,
    const f: F // not allowed because the type is a generic param
>(arg: A) -> R {
    f(arg)
}
```

Instead, this PR contains a workaround in `proc_macro::bridge::selfless_reify` (see its module-level comment for more details) that can provide something similar to the `ffi_wrapper` example above, but limited to `F` being `Copy` and ZST (and requiring an `F` value to prove the caller actually can create values of `F` and it's not uninhabited or some other unsound situation).

<hr/>

Hopefully this time we don't have a performance regression, and this has a chance to land.

cc `@mystor` `@bjorn3`
@nnethercote
Copy link
Contributor Author

Local measurements indicate that Make Buffer non-generic. is responsible for the perf regression, that's annoying.

nnethercote added a commit to nnethercote/rust that referenced this pull request May 29, 2022
It was made non-generic in rust-lang#97004, but that (surprisingly) caused a mild
performance regression.
@nnethercote
Copy link
Contributor Author

#97539 is the follow-up for the perf regression.

@pnkfelix
Copy link
Member

visiting for weekly performance triage. I won't mark this as triaged quite yet, since PR #97539 has not yet landed, but it certainly sounds like it is under control. Thanks @nnethercote !

nnethercote added a commit to nnethercote/rust that referenced this pull request May 31, 2022
This fixes a performance regression caused by making `Buffer`
non-generic in rust-lang#97004.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2022
…ods, r=eddyb

Inline `bridge::Buffer` methods.

This fixes a performance regression caused by making `Buffer`
non-generic in rust-lang#97004.

r? `@eddyb`
@nnethercote
Copy link
Contributor Author

The perf regression ended up being fixed in #97604.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Aug 3, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
proc_macro: don't pass a client-side function pointer through the server.

Before this PR, `proc_macro::bridge::Client<F>` contained both:
* the C ABI entry-point `run`, that the server can call to start the client
* some "payload" `f: F` passed to that entry-point
  * in practice, this was always a (client-side Rust ABI) `fn` pointer to the actual function the proc macro author wrote, i.e. `#[proc_macro] fn foo(input: TokenStream) -> TokenStream`

In other words, the client was passing one of its (Rust) `fn` pointers to the server, which was passing it back to the client, for the client to call (see later below for why that was ever needed).

I was inspired by `@nnethercote's` attempt to remove the `get_handle_counters` field from `Client` (see rust-lang/rust#97004 (comment)), which combined with removing the `f` ("payload") field, could theoretically allow for a `#[repr(transparent)]` `Client` that mostly just newtypes the C ABI entry-point `fn` pointer <sub>(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)</sub>.

<hr/>

So this PR removes that "payload". But it's not a simple refactor: the reason the field existed in the first place is because monomorphizing over a function type doesn't let you call the function without having a value of that type, because function types don't implement anything like `Default`, i.e.:
```rust
extern "C" fn ffi_wrapper<A, R, F: Fn(A) -> R>(arg: A) -> R {
    let f: F = ???; // no way to get a value of `F`
    f(arg)
}
```
That could be solved with something like this, if it was allowed:
```rust
extern "C" fn ffi_wrapper<
    A, R,
    F: Fn(A) -> R,
    const f: F // not allowed because the type is a generic param
>(arg: A) -> R {
    f(arg)
}
```

Instead, this PR contains a workaround in `proc_macro::bridge::selfless_reify` (see its module-level comment for more details) that can provide something similar to the `ffi_wrapper` example above, but limited to `F` being `Copy` and ZST (and requiring an `F` value to prove the caller actually can create values of `F` and it's not uninhabited or some other unsound situation).

<hr/>

Hopefully this time we don't have a performance regression, and this has a chance to land.

cc `@mystor` `@bjorn3`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants