[Incremental] Cache hashes for AdDef and ty::Slice<T> by wesleywiser · Pull Request #47373 · rust-lang/rust

wesleywiser · 2018-01-12T04:04:39Z

wesleywiser · 2018-01-12T04:06:14Z

src/librustc/ich/impls_ty.rs

Can you check this? I tried self as *const ty::Slice<T> as usize but got a casting error. The suggested fix in #41300 resolved the error but I'm not really sure why it works.

michaelwoerister · 2018-01-12T11:34:50Z

src/librustc/ty/mod.rs

Since AdtDefs are not recursive, you should be able to slightly optimize this by using HashMap::entry(). That avoids having to look up the key twice in the "cache miss" case.

You cannot do the same for ty::Slice because while hashing one slice, you might encounter a nested one somewhere down the line and consequently run into a RefCell double-borrow panic.

michaelwoerister · 2018-01-12T11:35:13Z

src/librustc/ty/mod.rs

I would move this whole block into the closure, after the "cache hit" case.

michaelwoerister · 2018-01-12T11:35:45Z

Right, a &ty::Slice<T> is a "fat pointer" because the [T] in the slice is of unknown size. When pointing to unsized things (like trait objects or slices) the compiler not only has to store the address of the thing but also its size/length in the pointer (which is what makes the pointer fat).

What you did should be correct but is a bit roundabout. First you are converting self which is a "fat" reference into a "fat" raw pointer (*const ty::Slice<T>) and that into a "thin" raw pointer (*const usize) and finally that into a usize.

I would suggest that you do the same thing as the Hash implementation for ty::Slice does:

rust/src/librustc/ty/mod.rs

Line 571 in 73ac5d6

(self.as_ptr(), self.len()).hash(s)

This makes use of the Deref implementation for ty::Slice. When calling as_ptr() on the self, the compiler will first auto-deref the ty::Slice<T> to a regular &[T] and then invoke as_ptr() on that. as_ptr() already returns a thin pointer (*const T), so we are fine.

Since the Hash implementation also takes the length of slice into account, I think we should do so too. It's possible that the interner stores some of these slices overlapped. That is, in theory, if you have two slices &[1,2,3] and &[1,2,3,4,5], you can store only the longer one and then point the shorter one to the same address, just with the shorter length. Not sure if that actually happens (if it does then the PartialEq implementation of ty::Slice is buggy [cc @eddyb]) but let's stay on the safe side.

So I would make the cache key (usize, usize) for address and length in the ty::Slice case, constructed simply by (self.as_ptr() as usize, self.len()).

eddyb · 2018-01-12T14:58:31Z

ty::Slice can't be sliced further, so the PartialEq impl shouldn't be problematic in practice.

wesleywiser · 2018-01-13T18:48:04Z

Thanks @michaelwoerister! I think I've addressed all of your concerns in the latest revisions.

michaelwoerister

Looks good! If you remove that redundant line I commented on below, this should be ready for a performance test.

michaelwoerister · 2018-01-15T11:58:15Z

src/librustc/ty/mod.rs

This line seems to be a leftover from something.

Oops, I'm not even sure where that came from lol

Part of rust-lang#47294

Fixes rust-lang#47294

wesleywiser · 2018-01-16T01:18:10Z

Fixed

michaelwoerister · 2018-01-16T09:48:37Z

Excellent. I'll start a try-build so we can measure the performance impact.

@bors try

bors · 2018-01-16T09:48:47Z

⌛ Trying commit 45bd091 with merge 1304b66...

@michaelwoerister

[Incremental] Cache hashes for AdDef and ty::Slice<T> r? @michaelwoerister

bors · 2018-01-16T11:15:11Z

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

Mark-Simulacrum · 2018-01-16T13:02:29Z

Perf queued.

michaelwoerister · 2018-01-16T16:50:17Z

perf.rlo link (~~doesn't work yet~~ works now)

michaelwoerister · 2018-01-17T11:25:07Z

Nice! These are some pretty good performance improvements. If it doesn't cause any trouble with further testing, we might even want to backport it to the current beta, given how small the change set is. Thank you, @wesleywiser!

@bors r+

bors · 2018-01-17T11:25:08Z

📌 Commit 45bd091 has been approved by michaelwoerister

wesleywiser · 2018-01-17T14:38:53Z

Thanks @michaelwoerister!

bors · 2018-01-22T23:32:22Z

⌛ Testing commit 45bd091 with merge 47a8eb7...

@michaelwoerister

…ster [Incremental] Cache hashes for AdDef and ty::Slice<T> r? @michaelwoerister

bors · 2018-01-23T02:26:35Z

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

michaelwoerister · 2018-01-23T09:25:09Z

Very good numbers for a single optimization :)

http://perf.rust-lang.org/compare.html?start=ae920dcc98c9b18b38aac03367f7f1cd6dce7d2d&end=47a8eb7c4e24b61a8a9ab4eaff60ef65291aaa56&stat=instructions%3Au

rust-highfive assigned michaelwoerister Jan 12, 2018

wesleywiser commented Jan 12, 2018

View reviewed changes

michaelwoerister reviewed Jan 12, 2018

View reviewed changes

src/librustc/ty/mod.rs Outdated

Copy link

Member

michaelwoerister Jan 12, 2018

Choose a reason for hiding this comment

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

I would move this whole block into the closure, after the "cache hit" case.

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 13, 2018

wesleywiser force-pushed the incr_cache_hashing branch from c8adecd to eab3bfc Compare January 13, 2018 18:47

michaelwoerister reviewed Jan 15, 2018

View reviewed changes

wesleywiser added 2 commits January 15, 2018 20:16

[incremental] Cache AdtDef hashes

14780b3

Part of rust-lang#47294

[incremental] Cache ty::Slice<T> hashes

45bd091

Fixes rust-lang#47294

wesleywiser force-pushed the incr_cache_hashing branch from eab3bfc to 45bd091 Compare January 16, 2018 01:18

bors added a commit that referenced this pull request Jan 16, 2018

Auto merge of #47373 - wesleywiser:incr_cache_hashing, r=<try>

1304b66

[Incremental] Cache hashes for AdDef and ty::Slice<T> r? @michaelwoerister

carols10cents 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 Jan 22, 2018

bors added a commit that referenced this pull request Jan 22, 2018

Auto merge of #47373 - wesleywiser:incr_cache_hashing, r=michaelwoeri…

47a8eb7

…ster [Incremental] Cache hashes for AdDef and ty::Slice<T> r? @michaelwoerister

bors merged commit 45bd091 into rust-lang:master Jan 23, 2018

bors mentioned this pull request Jan 23, 2018

incr.comp.: Cache type_of and some other queries. #47455

Merged

Uh oh!

Conversation

wesleywiser commented Jan 12, 2018

Uh oh!

wesleywiser Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelwoerister Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

michaelwoerister Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Jan 12, 2018

Uh oh!

eddyb commented Jan 12, 2018

Uh oh!

wesleywiser commented Jan 13, 2018

Uh oh!

michaelwoerister left a comment

Choose a reason for hiding this comment

Uh oh!

michaelwoerister Jan 15, 2018

Choose a reason for hiding this comment

Uh oh!

wesleywiser Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Jan 16, 2018

Uh oh!

michaelwoerister commented Jan 16, 2018

Uh oh!

bors commented Jan 16, 2018

Uh oh!

bors commented Jan 16, 2018

Uh oh!

Mark-Simulacrum commented Jan 16, 2018

Uh oh!

michaelwoerister commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelwoerister commented Jan 17, 2018

Uh oh!

bors commented Jan 17, 2018

Uh oh!

wesleywiser commented Jan 17, 2018

Uh oh!

bors commented Jan 22, 2018

Uh oh!

bors commented Jan 23, 2018

Uh oh!

michaelwoerister commented Jan 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wesleywiser Jan 12, 2018 •

edited

Loading

michaelwoerister commented Jan 16, 2018 •

edited

Loading