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

Break up __pin_project_internal #71

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

nnethercote
Copy link
Contributor

I have been investigating expensive declarative macros recently. pin_project_lite::pin_project showed up as expensive for the futures-lite crate. This macro is very large, with 47 rules, many of these being internal rules. I thought that splitting it up into lots of smaller macros might help with performance. It only made a small (2%) performance improvement, but I think it increases readability enough that it's worth doing anyway. So I made this pull request.

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks! It's interesting that this improves performance. (IIRC, in proc-macro, it tended to take longer to compile the more macros there were. I thought the declarative macro had similar nature.)

As for CI failure, I think it can probably be fixed by rebasing.

@taiki-e taiki-e changed the title Break up __pin project internal Break up __pin_project_internal Apr 19, 2022
This isn't useful by itself, but it sets things up for the next commit,
which will break `__pin_project_internal!` into a number of smaller
macros.
By removal internal rules, this commit turns one giant macro with 47
rules into 23 macros with a small number of rules each. I find the
resulting code *much* easier to read; I found the original giant macro
quite overwhelming. The new code also needs fewer comments because the
code is clearer.

The commit also fixes some minor formatting inconsistencies, such as
always putting a space between the `!` and the `{` in a macro
invocation, and using parens instead of braces for a few matchers.
@nnethercote
Copy link
Contributor Author

It improves performance because it avoids lots of rule matching attempts that cannot succeed.

@nnethercote
Copy link
Contributor Author

I just upgraded to the latest Nightly and ran cargo +nightly --all --all-features. For the ui tests I got lots of messages about expected output vs actual output, but no test failures. Those were just marked as wip... "work in progress"??

That command also changed lots of .stderr files, which was unexpected. A lot of the changes are due to the macro name changes, e.g.:

-  = note: this error originates in the macro `$crate::__pin_project_internal` (in Nightly builds, run with -Z macro-backtrace for more info)
+  = note: this error originates in the macro `$crate::__pin_project_make_drop_impl` (in Nightly builds, run with -Z macro-backtrace for more info)

but there are also some like this, which I don't understand:

-error: no rules expected the token `[`
- --> tests/ui/pin_project/invalid-bounds.rs:3:1
+error: no rules expected the token `:`
+ --> tests/ui/pin_project/invalid-bounds.rs:4:33
   |
-3 | / pin_project! {
-4 | |     struct Generics1<T: 'static : Sized> { //~ ERROR no rules expected the token `:`
-5 | |         field: T,
-6 | |     }
-7 | | }
-  | |_^ no rules expected this token in macro call
-  |
-  = note: this error originates in the macro `$crate::__pin_project_internal` (in Nightly builds, run with -Z macro-backtrace for more info)
+4 |     struct Generics1<T: 'static : Sized> { //~ ERROR no rules expected the token `:`
+  |                                 ^ no rules expected this token in macro call

@nnethercote nnethercote force-pushed the break-up-__pin_project_internal branch from dc21c09 to 8b97d80 Compare April 19, 2022 02:38
@taiki-e
Copy link
Owner

taiki-e commented Apr 21, 2022

For the ui tests I got lots of messages about expected output vs actual output, but no test failures.

Locally, ui test output is blessed by default.

but there are also some like this, which I don't understand:

The new diagnosis seems correct. I guess the rule reordering or rule separation may have changed the tokens that are considered expected to be matched first.

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 21, 2022

Build succeeded:

@bors bors bot merged commit d793ebf into taiki-e:main Apr 21, 2022
@nnethercote nnethercote deleted the break-up-__pin_project_internal branch April 21, 2022 20:34
@nnethercote
Copy link
Contributor Author

Thanks!

@taiki-e
Copy link
Owner

taiki-e commented Apr 26, 2022

Published in 0.2.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants