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.

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

Use a hash-table when selecting impls #24965

Merged
merged 3 commits into from May 1, 2015

Conversation

Projects
None yet
7 participants
@arielb1
Contributor

arielb1 commented Apr 29, 2015

This uses a (per-trait) hash-table to separate impls from different TraitDefs, and makes coherence go so much quicker. I will post performance numbers tomorrow.

This is still WIP, as when there's an overlap error, impls can get printed in the wrong order, which causes a few issues. Should I pick the local impl with the smallest NodeId to print?

Could you take a look at this @nikomatsakis?

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Apr 29, 2015

Collaborator

r? @pnkfelix

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

Collaborator

rust-highfive commented Apr 29, 2015

r? @pnkfelix

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

arielb1 added some commits Apr 21, 2015

Stop using Rc in TraitRef and TraitDef
The former stopped making sense when we started interning substs and made
TraitRef a 2-word copy type, and I'm moving the latter into an arena as
they live as long as the type context.
Use hash-tables in trait selection
Puts implementations in bins hashed by the fast-reject key, and
only looks up the relevant impls, reducing O(n^2)-ishness

Before: 688.92user 5.08system 8:56.70elapsed 129%CPU (0avgtext+0avgdata 1208164maxresident)k, LLVM 379.142s
After: 637.78user 5.11system 8:17.48elapsed 129%CPU (0avgtext+0avgdata 1201448maxresident)k LLVM 375.552s

Performance increase is +7%-ish

@arielb1 arielb1 changed the title from [WIP] Use a hash-table when selecting impls to Use a hash-table when selecting impls Apr 30, 2015

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1
Contributor

arielb1 commented Apr 30, 2015

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Nice, I've been meaning to do this.

nikomatsakis commented on bd1f734 Apr 30, 2015

Nice, I've been meaning to do this.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

I think a comment clarifying what you mean by blanket would be helpful

I think a comment clarifying what you mean by blanket would be helpful

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Nit: It'd be good to say something like "keyed by the simplified version of the Self type"

Nit: It'd be good to say something like "keyed by the simplified version of the Self type"

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Nit: can we assert that this is always set consistently?

Nit: can we assert that this is always set consistently?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Nit: can you write these in a chained style with the . lined up? maybe with an intermediate variable if necessary? I find this line pretty dense as is.

Nit: can you write these in a chained style with the . lined up? maybe with an intermediate variable if necessary? I find this line pretty dense as is.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

nit: similarly here I'd prefer a chain, but it's also ok as is

nit: similarly here I'd prefer a chain, but it's also ok as is

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

I think this method should just take the self type. Then you don't have to worry about it being none, and it'd probably be easier to integrate into method dispatch, no?

I think this method should just take the self type. Then you don't have to worry about it being none, and it'd probably be easier to integrate into method dispatch, no?

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Apr 30, 2015

Owner

I have to worry about simplify(self) being none anyway.

Owner

arielb1 replied Apr 30, 2015

I have to worry about simplify(self) being none anyway.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

The self-type should never be None in a trait-ref, I don't think. It is only None in object types. (But see above.)

The self-type should never be None in a trait-ref, I don't think. It is only None in object types. (But see above.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

can you elaborate? what method? what list?

can you elaborate? what method? what list?

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Apr 30, 2015

Owner

You wrote this comment (at 6349a61), not me.

Owner

arielb1 replied Apr 30, 2015

You wrote this comment (at 6349a61), not me.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Nit: can you move the { to the next line, or otherwise reformat so that the if body is separated from the condition?

Nit: can you move the { to the next line, or otherwise reformat so that the if body is separated from the condition?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

This looks great too. I like how you pulled more things into TraitDef. I left some comments but am basically happy.

nikomatsakis commented on 7ae4a8e Apr 30, 2015

This looks great too. I like how you pulled more things into TraitDef. I left some comments but am basically happy.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Contributor

@arielb1

This is still WIP, as when there's an overlap error, impls can get printed in the wrong order, which causes a few issues. Should I pick the local impl with the smallest NodeId to print?

I see that you chose to pick the smallest NodeId, which seems fine -- do you feel this resolves your concern here? (It's not clear to me that there is a "wrong" order, but it'd be easiest and best to stick with the same order as the previous code, I agree, and a consistent order is good for testing.)

Contributor

nikomatsakis commented Apr 30, 2015

@arielb1

This is still WIP, as when there's an overlap error, impls can get printed in the wrong order, which causes a few issues. Should I pick the local impl with the smallest NodeId to print?

I see that you chose to pick the smallest NodeId, which seems fine -- do you feel this resolves your concern here? (It's not clear to me that there is a "wrong" order, but it'd be easiest and best to stick with the same order as the previous code, I agree, and a consistent order is good for testing.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Contributor

OK, so, r+ but I left a few nits and other questions. Nothing egregious, r=me when you've fixed them (or, if there are things you don't agree with, let me know).

Contributor

nikomatsakis commented Apr 30, 2015

OK, so, r+ but I left a few nits and other questions. Nothing egregious, r=me when you've fixed them (or, if there are things you don't agree with, let me know).

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 30, 2015

Good question :) I don't remember if there is a good reason or... it was just written that way.

nikomatsakis replied Apr 30, 2015

Good question :) I don't remember if there is a good reason or... it was just written that way.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Apr 30, 2015

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Apr 30, 2015

Member

I will post performance numbers tomorrow.

Out of interest, did you end up collecting some?

Member

huonw commented Apr 30, 2015

I will post performance numbers tomorrow.

Out of interest, did you end up collecting some?

@luqmana

This comment has been minimized.

Show comment
Hide comment
@luqmana

luqmana May 1, 2015

Member

@huonw: From the second commit message:

Puts implementations in bins hashed by the fast-reject key, and
only looks up the relevant impls, reducing O(n^2)-ishness

Before: 688.92user 5.08system 8:56.70elapsed 129%CPU (0avgtext+0avgdata 1208164maxresident)k, LLVM 379.142s
After: 637.78user 5.11system 8:17.48elapsed 129%CPU (0avgtext+0avgdata 1201448maxresident)k LLVM 375.552s

Performance increase is +7%-ish
Member

luqmana commented May 1, 2015

@huonw: From the second commit message:

Puts implementations in bins hashed by the fast-reject key, and
only looks up the relevant impls, reducing O(n^2)-ishness

Before: 688.92user 5.08system 8:56.70elapsed 129%CPU (0avgtext+0avgdata 1208164maxresident)k, LLVM 379.142s
After: 637.78user 5.11system 8:17.48elapsed 129%CPU (0avgtext+0avgdata 1201448maxresident)k LLVM 375.552s

Performance increase is +7%-ish

bors added a commit that referenced this pull request May 1, 2015

Auto merge of #24965 - arielb1:instant-reject, r=nikomatsakis
This uses a (per-trait) hash-table to separate impls from different TraitDefs, and makes coherence go so much quicker. I will post performance numbers tomorrow.

This is still WIP, as when there's an overlap error, impls can get printed in the wrong order, which causes a few issues. Should I pick the local impl with the smallest NodeId to print?

Could you take a look at this @nikomatsakis?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 1, 2015

Contributor

⌛️ Testing commit 30a5448 with merge aecf3d8...

Contributor

bors commented May 1, 2015

⌛️ Testing commit 30a5448 with merge aecf3d8...

@bors bors merged commit 30a5448 into rust-lang:master May 1, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment