Skip to content

Only lookup types in one interner#50332

Merged
bors merged 1 commit into
rust-lang:masterfrom
Zoxc:interner-split
May 11, 2018
Merged

Only lookup types in one interner#50332
bors merged 1 commit into
rust-lang:masterfrom
Zoxc:interner-split

Conversation

@Zoxc

@Zoxc Zoxc commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

No description provided.

@rust-highfive

rust-highfive commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

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

Zoxc commented Apr 30, 2018

Copy link
Copy Markdown
Contributor Author

@bors try

@bors

bors commented Apr 30, 2018

Copy link
Copy Markdown
Collaborator

⌛ 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

bors commented Apr 30, 2018

Copy link
Copy Markdown
Collaborator

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

@petrochenkov

petrochenkov commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Apr 30, 2018
@Zoxc

Zoxc commented Apr 30, 2018

Copy link
Copy Markdown
Contributor Author

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

@Mark-Simulacrum

Mark-Simulacrum commented Apr 30, 2018

Copy link
Copy Markdown
Member

Perf queued (and started).

@Zoxc

Zoxc commented May 1, 2018

Copy link
Copy Markdown
Contributor Author

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

eddyb
eddyb reviewed May 1, 2018
View reviewed changes
Comment thread src/librustc/ty/context.rs Outdated

@eddyb eddyb May 1, 2018

Copy link
Copy Markdown
Contributor

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
Comment thread src/librustc/ty/flags.rs Outdated

@eddyb eddyb May 1, 2018

Copy link
Copy Markdown
Contributor

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

eddyb commented May 1, 2018

Copy link
Copy Markdown
Contributor

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

Zoxc commented May 2, 2018

Copy link
Copy Markdown
Contributor Author

@bors try

@bors

bors commented May 2, 2018

Copy link
Copy Markdown
Collaborator

⌛ 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

bors commented May 2, 2018

Copy link
Copy Markdown
Collaborator

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

@Zoxc

Zoxc commented May 2, 2018

Copy link
Copy Markdown
Contributor Author

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

@Mark-Simulacrum

Mark-Simulacrum commented May 2, 2018

Copy link
Copy Markdown
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

kennytm commented May 4, 2018

Copy link
Copy Markdown
Member

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

@kennytm

kennytm commented May 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@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

Zoxc commented May 7, 2018

Copy link
Copy Markdown
Contributor Author

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

michaelwoerister commented May 11, 2018

Copy link
Copy Markdown
Member

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

@bors r+

@bors

bors commented May 11, 2018

Copy link
Copy Markdown
Collaborator

📌 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

bors commented May 11, 2018

Copy link
Copy Markdown
Collaborator

⌛ 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

bors commented May 11, 2018

Copy link
Copy Markdown
Collaborator

☀️ 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

1 more reviewer

@eddyb eddyb eddyb left review comments

Reviewers whose approvals may not affect merge requirements

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