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

Only lookup types in one interner #50332

Merged
merged 1 commit into from
May 11, 2018
Merged

Only lookup types in one interner #50332

merged 1 commit into from
May 11, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Apr 30, 2018

No description provided.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 30, 2018

@bors try

@bors
Copy link
Contributor

bors commented Apr 30, 2018

⌛ Trying commit 3d038cc with merge 0f8ec0a...

bors added a commit that referenced this pull request Apr 30, 2018
@bors
Auto merge of #50332 - Zoxc:interner-split, r=<try>
0f8ec0a 
[WIP] Only lookup types in one interner
@bors
Copy link
Contributor

bors commented Apr 30, 2018

☀️ Test successful - status-travis
State: approved= try=True

@petrochenkov
Copy link
Contributor

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Apr 30, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 30, 2018

@Mark-Simulacrum I'd like a perf run here

@Mark-Simulacrum
Copy link
Member

Perf queued (and started).

@Zoxc
Copy link
Contributor Author

Zoxc commented May 1, 2018

Results: http://perf.rust-lang.org/compare.html?start=9ff8ec8fdf02b6d8ba35373943e0c6de13597697&end=0f8ec0a707d11fe3ffeccd1e4bafe9ca8922b42c&stat=instructions%3Au

eddyb
eddyb reviewed May 1, 2018
View reviewed changes
src/librustc/ty/context.rs Outdated
let flags = super::flags::FlagComputation::for_sty(&st);

if flags.flags.intersects(ty::TypeFlags::KEEP_IN_LOCAL_TCX) != true {
eprintln!("MISMATCH TRUE {:?}", st);
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to remove these eprintln!s? You already have asserts here.

eddyb
eddyb reviewed May 1, 2018
View reviewed changes
src/librustc/ty/flags.rs Outdated
@@ -250,3 +250,160 @@ impl FlagComputation {
}
}
}

pub(crate) fn sty_in_local_arena(st: &ty::TypeVariants) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Can't you reuse TypeVisitor for this? Seems like a lot of code IMO.

@rust-highfive

This comment has been minimized.

Sign in to view
@eddyb
Copy link
Member

eddyb commented May 1, 2018

LGTM, modulo the implementation duplicating a lot of logic.

@Zoxc Zoxc force-pushed the interner-split branch from fcb43fb to 1fc4ad7 Compare May 1, 2018 12:49
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@Zoxc Zoxc force-pushed the interner-split branch from 239c586 to d452c3e Compare May 2, 2018 10:11
@Zoxc
Copy link
Contributor Author

Zoxc commented May 2, 2018

@bors try

@bors
Copy link
Contributor

bors commented May 2, 2018

⌛ Trying commit d452c3e with merge 116cf1c...

bors added a commit that referenced this pull request May 2, 2018
@bors
Auto merge of #50332 - Zoxc:interner-split, r=<try>
116cf1c 
[WIP] Only lookup types in one interner
@bors
Copy link
Contributor

bors commented May 2, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Zoxc
Copy link
Contributor Author

Zoxc commented May 2, 2018

@Mark-Simulacrum I'd like another perf run to see if the fast path helps

@Mark-Simulacrum
Copy link
Member

Perf queued.

@kennytm kennytm added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 3, 2018
@kennytm
Copy link
Member

kennytm commented May 4, 2018

Perf link when it is available: http://perf.rust-lang.org/compare.html?start=6a87289fa4f49b6bdd62f33f69a580026223421f&end=116cf1c0ba1e36f4441db6653e871bc8d3d3f670&stat=instructions%3Au

@kennytm
Copy link
Member

kennytm commented May 6, 2018 •
edited
Loading

@Zoxc @eddyb Perf result is now available.

@bors try- clean

@kennytm kennytm removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 6, 2018
@shepmaster shepmaster 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 May 6, 2018
@Zoxc Zoxc force-pushed the interner-split branch from d452c3e to 9fe197d Compare May 7, 2018 11:11
@Zoxc Zoxc changed the title [WIP] Only lookup types in one interner Only lookup types in one interner May 7, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented May 7, 2018

It seems like the fast-path wasn't a win. I've removed it and also applied the transformation to the interning macro.

@Zoxc Zoxc force-pushed the interner-split branch from 9fe197d to e245d69 Compare May 7, 2018 14:40
@michaelwoerister
Copy link
Member

lgtm! You can still r- if you have any concerns, @eddyb.

@bors r+

@bors
Copy link
Contributor

bors commented May 11, 2018

📌 Commit e245d69 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 11, 2018
@bors
Copy link
Contributor

bors commented May 11, 2018

⌛ Testing commit e245d69 with merge fe63e47...

bors added a commit that referenced this pull request May 11, 2018
@bors
Auto merge of #50332 - Zoxc:interner-split, r=michaelwoerister
fe63e47 
Only lookup types in one interner
@bors
Copy link
Contributor

bors commented May 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing fe63e47 to master...

@bors bors merged commit e245d69 into rust-lang:master May 11, 2018
@Zoxc Zoxc deleted the interner-split branch May 11, 2018 15:44
This was referenced May 11, 2018
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Zoxc @rust-highfive @bors @petrochenkov @Mark-Simulacrum @eddyb @kennytm @michaelwoerister @shepmaster