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

Have Derive Attribute share a token tree with it's proc macros. #16835

Merged
merged 8 commits into from
Mar 21, 2024

Conversation

wyatt-herkamp
Copy link
Contributor

@wyatt-herkamp wyatt-herkamp commented Mar 14, 2024

The goal of this PR is to stop creating a token tree for each derive proc macro.

This is done by giving the derive proc macros an id to its parent derive element.

From running the analysis stat on the rust analyzer project I did see a small memory decrease.

Inference:           42.80s, 362ginstr, 591mb
MIR lowering:        8.67s, 67ginstr, 291mb
Mir failed bodies: 18 (0%)
Data layouts:        85.81ms, 609minstr, 8mb
Failed data layouts: 135 (6%)
Const evaluation:    440.57ms, 5235minstr, 13mb
Failed const evals: 1 (0%)
Total:               64.16s, 552ginstr, 1731mb

After Change

Inference:           40.32s, 340ginstr, 593mb
MIR lowering:        7.95s, 62ginstr, 292mb
Mir failed bodies: 18 (0%)
Data layouts:        87.97ms, 591minstr, 8mb
Failed data layouts: 135 (6%)
Const evaluation:    433.38ms, 5226minstr, 14mb
Failed const evals: 1 (0%)
Total:               60.49s, 523ginstr, 1680mb

Currently this breaks the expansion for the actual derive attribute.

TODO

  • Pick a better name for the function smart_macro_arg

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2024
@bors
Copy link
Contributor

bors commented Mar 15, 2024

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

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

That's a smart idea, re-using the #[derive] attribute for the query, since all the inner derives should share the arg! Though this raises the question what to do about the "pseudo" expansion that we use this for right now... Guess it's time to move that hack into a different layer, the question is how

crates/hir-expand/src/db.rs Outdated Show resolved Hide resolved
crates/hir-expand/src/db.rs Outdated Show resolved Hide resolved
crates/hir-expand/src/db.rs Outdated Show resolved Hide resolved
@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2024
@wyatt-herkamp
Copy link
Contributor Author

That's a smart idea, re-using the #[derive] attribute for the query, since all the inner derives should share the arg!

Thank you! I have changed how I did this though I forgot to commit the code. I will have that up soon.

Though this raises the question what to do about the "pseudo" expansion that we use this for right now... Guess it's time to move that hack into a different layer, the question is how

So I was wanting to add a new setting and stuff.

The default would be
If you request the expansion on the #[derive] it would just return the struct but with all the "fixups" applied. Including see what cfg_attr go are removed and what is applied

The second would be collect all the derives and return them as one expansion.

This would only really matter for the rust-analyzer/expandMacro command or request. We would need to add seperate handling for this under expand_macro

@wyatt-herkamp wyatt-herkamp marked this pull request as ready for review March 18, 2024 11:10
@wyatt-herkamp
Copy link
Contributor Author

wyatt-herkamp@8b78a18 adds a new MacroCallKind DeriveAttr

The reason I did this is because handling cfg/cfg_attr and other derive related censors. This also allows us to go about handling expansion requests on the derive attribute a bit easier. We also can only apply it on an Adt(Struct, Enum, Union).

@wyatt-herkamp
Copy link
Contributor Author

@Veykril should any tests be added for this PR?

@Veykril
Copy link
Member

Veykril commented Mar 18, 2024

Don't think there is an opportunity of testing here, as the only effect this has is reduce memory usage

crates/hir-expand/src/db.rs Outdated Show resolved Hide resolved
crates/hir-expand/src/db.rs Outdated Show resolved Hide resolved
crates/hir-expand/src/lib.rs Outdated Show resolved Hide resolved
@Veykril Veykril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-decision and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 18, 2024
@bors
Copy link
Contributor

bors commented Mar 21, 2024

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

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

speculative_expand also needs to call the new macro arg stuff

crates/hir-def/src/nameres/collector.rs Outdated Show resolved Hide resolved
crates/hir-def/src/nameres/collector.rs Outdated Show resolved Hide resolved
crates/hir-def/src/nameres/collector.rs Outdated Show resolved Hide resolved
crates/hir-expand/src/db.rs Outdated Show resolved Hide resolved
crates/hir-expand/src/db.rs Show resolved Hide resolved
crates/hir-expand/src/db.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Mar 21, 2024

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2024

📌 Commit c381d0f has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 21, 2024

⌛ Testing commit c381d0f with merge 16c8dee...

@bors
Copy link
Contributor

bors commented Mar 21, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 16c8dee to master...

@bors bors merged commit 16c8dee into rust-lang:master Mar 21, 2024
11 checks passed
@wyatt-herkamp wyatt-herkamp deleted the use_one_tt_for_a_derive branch March 21, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-decision S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants