Skip to content

Try to reduce Semantics monomorphisations #5162

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
Merged
merged 2 commits into from
Jul 1, 2020
Merged

Try to reduce Semantics monomorphisations #5162

merged 2 commits into from
Jul 1, 2020

Conversation

@lnicola
Copy link
Member

@lnicola lnicola commented Jul 1, 2020

No description provided.

@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020

This helps less than expected, but still seems like a decent win:

image

And it's not visibly slower:

Database loaded 1.255206199s
Crates in this dir: 36
Total modules found: 489
Total declarations: 9541
Total functions: 7431
Item Collection: 18.003271618s, 0b allocated 0b resident
Total expressions: 194020                                                                                                                          
Expressions of unknown type: 1588 (0%)
Expressions of partially unknown type: 992 (0%)
Type mismatches: 452
Inference: 29.650529527s, 0b allocated 0b resident
Total: 47.653807827s, 0b allocated 0b resident

Database loaded 254.772917ms
Crates in this dir: 36
Total modules found: 489
Total declarations: 9541
Total functions: 7431
Item Collection: 17.795208032s, 0b allocated 0b resident
Total expressions: 194020                                                                                                                          
Expressions of unknown type: 1576 (0%)
Expressions of partially unknown type: 980 (0%)
Type mismatches: 452
Inference: 28.918111862s, 0b allocated 0b resident
Total: 46.713327311s, 0b allocated 0b resident

matklad
matklad reviewed Jul 1, 2020
View reviewed changes
crates/ra_hir/src/semantics.rs
impl_: SemanticsImpl<'db>,
}

pub struct SemanticsImpl<'db> {
Copy link
Contributor

@matklad matklad Jul 1, 2020

Choose a reason for hiding this comment

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

Does it need to be pub?

Copy link
Member Author

@lnicola lnicola Jul 1, 2020

Choose a reason for hiding this comment

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

I'm not sure. It's now used in the ToDef trait and I changed that to take SemanticsImpl.

Copy link
Contributor

@matklad matklad Jul 1, 2020

Choose a reason for hiding this comment

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

Ah, right, than let's keep it pub for simplicity

crates/ra_hir/src/semantics.rs Outdated
node: &SyntaxNode,
offset: TextSize,
) -> Option<N> {
self.impl_.find_node_at_offset_with_descend(node, offset)
Copy link
Contributor

@matklad matklad Jul 1, 2020

Choose a reason for hiding this comment

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

Let's inline generic parts of impl_.find_node_at_offset_with_descend here, and leave only non-generic bit in SemanticsImpl here and elsewhere .

Copy link
Member Author

@lnicola lnicola Jul 1, 2020

Choose a reason for hiding this comment

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

I think that will turn the non-generic bits return impl Iterator<Item=SyntaxNode so that the generic ones can call find_map(N::cast) on them.

Copy link
Contributor

@matklad matklad Jul 1, 2020

Choose a reason for hiding this comment

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

Right, returning, returning an impl Trait is OK, as that's still a specific type under the hood.

@lnicola lnicola changed the title Try to make reduce Semantics monomorphisations Try to reduce Semantics monomorphisations Jul 1, 2020
@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020

r? @matklad

@matklad
Copy link
Contributor

matklad commented Jul 1, 2020

bors d+

there's a bunch of .js changes somehow

@bors
Copy link
Contributor

bors bot commented Jul 1, 2020

✌️ lnicola can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020 •
edited
Loading

Sorry, not sure what happened there.

bors r=matklad

@bors
Copy link
Contributor

bors bot commented Jul 1, 2020

✌️ matklad can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020

bors r=matklad

@bors
Copy link
Contributor

bors bot commented Jul 1, 2020

Build succeeded:

@bors bors bot merged commit 248b656 into rust-lang:master Jul 1, 2020
@lnicola lnicola deleted the mono-semantics branch July 1, 2020 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@matklad matklad matklad left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lnicola @matklad