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.

Sign up for GitHub

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

Jump to bottom

internal: Use triomphe::Arc #14718

Merged
merged 1 commit into from May 2, 2023
Merged

internal: Use triomphe::Arc #14718

merged 1 commit into from May 2, 2023

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented May 2, 2023 •
edited

Closes #14717

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2023
lnicola
lnicola commented May 2, 2023
View reviewed changes
crates/hir-expand/src/attrs.rs
@@ -78,8 +81,9 @@ impl RawAttrs {
<< AttrId::AST_INDEX_BITS;
it
}))
.collect(),
),
// FIXME: use `Arc::from_iter` when it becomes available
Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to fix these up.

lnicola
lnicola commented May 2, 2023
View reviewed changes
crates/base-db/src/fixture.rs
@@ -334,7 +335,7 @@ pub fn identity(_attr: TokenStream, item: TokenStream) -> TokenStream {
ProcMacro {
name: "identity".into(),
kind: crate::ProcMacroKind::Attr,
expander: Arc::new(IdentityProcMacroExpander),
expander: sync::Arc::new(IdentityProcMacroExpander),
Copy link
Member Author

Choose a reason for hiding this comment

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

triomphe::Arc doesn't support trait objects

@lnicola lnicola marked this pull request as draft May 2, 2023 14:16
@lnicola lnicola force-pushed the triomphe branch 3 times, most recently from 5c2e2d9 to 8ae7085 Compare May 2, 2023 14:40
@bors
Copy link
Collaborator

bors commented May 2, 2023

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

@Veykril
Copy link
Member

Veykril commented May 2, 2023

Nice to see that it mostly just works as a drop-in replacement

@lnicola lnicola marked this pull request as ready for review May 2, 2023 17:05
@lnicola
Copy link
Member Author

lnicola commented May 2, 2023

Nice to see that it mostly just works as a drop-in replacement

😬

Think we can merge this now? It might tend to bitrot. I can fix the FIXMEs when the next triomphe release gets out.

We could also use ThinArc in some places (there's at least one FIXME for it), but I'd rather do that in another PR.

@HKalbasi
Copy link
Member

HKalbasi commented May 2, 2023

Is 4MB worth this (or, how many megabytes worth it)? I guess it will create a non zero amount of headache in future (importing the wrong arc, or mixing arcs by mistake which ends up in using std's arc half of the places, and some things similar)

@Veykril
Copy link
Member

Veykril commented May 2, 2023 •
edited

We are forced to use std's Arc by chalk in some places I believe, other than that once we can make use of clippy properly on this repo we can forbid the std one to be used. I think it should be fine then.

Also of note regarding memory usage. 4Mb might be more when IDE features have been used, since analysis stats only really tests type inference + body lowering memory usage and not much more.

@lnicola
Copy link
Member Author

lnicola commented May 2, 2023

And ThinArc might bring some extra wins.

@HKalbasi
Copy link
Member

HKalbasi commented May 2, 2023

other than that once we can make use of clippy properly on this repo we can forbid the std one to be used

hmm, then it would be good enough I think.

Also of note regarding memory usage. 4Mb might be more when IDE features have been used, since analysis stats only really tests type inference + body lowering memory usage and not much more.

Maybe we can measure it by using lsif instead of analysis stats?

@lnicola
Copy link
Member Author

lnicola commented May 2, 2023 •
edited 

# baseline
76.42user 0.78system 1:17.47elapsed 99%CPU (0avgtext+0avgdata 1692104maxresident)k

# pr
74.30user 0.68system 1:15.27elapsed 99%CPU (0avgtext+0avgdata 1688284maxresident)k

So just 3.73 MB :-(.

@Veykril
Copy link
Member

Veykril commented May 2, 2023

I don't think lsif will test much else, that also just typechecks the project to resolve definitions.

@lnicola lnicola changed the title Use triomphe::Arc internal: Use triomphe::Arc May 2, 2023
@HKalbasi
Copy link
Member

HKalbasi commented May 2, 2023

It also executes hover on everything. Do you mean an specific ide feature, or the general usage within ide (changing files, ...)?

@Veykril
Copy link
Member

Veykril commented May 2, 2023

general usage over time for ide features ye

@Veykril
Copy link
Member

Veykril commented May 2, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented May 2, 2023

📌 Commit 7197a27 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 2, 2023

⌛ Testing commit 7197a27 with merge 9811a3a...

@bors
Copy link
Collaborator

bors commented May 2, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 9811a3a to master...

@bors bors merged commit 9811a3a into rust-lang:master May 2, 2023
10 checks passed
@lnicola lnicola deleted the triomphe branch May 3, 2023 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider replacing std::sync::Arc usages with an Arc implementation that has no weak support
5 participants
@lnicola @bors @Veykril @HKalbasi @rustbot