Queryify Vtable methods #45137

theotherjimmy · 2017-10-09T15:01:07Z

This query might come with a downside: It converts an iterator to a Vec, which may increase the working set of rustc on programs that use many many traits (I think that's where this is used).

rust-highfive · 2017-10-09T15:01:19Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis

Looks good! Left a few minor suggestions.

nikomatsakis · 2017-10-09T15:34:24Z

src/librustc/ty/maps/mod.rs

@@ -227,6 +227,8 @@ define_maps! { <'tcx>
    [] fn item_body_nested_bodies: ItemBodyNestedBodies(DefId) -> ExternBodyNestedBodies,
    [] fn const_is_rvalue_promotable_to_static: ConstIsRvaluePromotableToStatic(DefId) -> bool,
    [] fn is_mir_available: IsMirAvailable(DefId) -> bool,
+    [] fn vtable_methods: vtable_methods_node(ty::PolyTraitRef<'tcx>)
+                          -> Vec<Option<(DefId, &'tcx Substs<'tcx>)>>,


Returning Rc<Vec<Option<..>>> would be better.

Right! I had that change before, I should re-apply that patch.

nikomatsakis · 2017-10-09T15:34:45Z

src/librustc/traits/mod.rs

@@ -650,12 +650,12 @@ pub fn normalize_and_test_predicates<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
 /// Given a trait `trait_ref`, iterates the vtable entries
 /// that come from `trait_ref`, including its supertraits.
 #[inline] // FIXME(#35870) Avoid closures being unexported due to impl Trait.
-pub fn get_vtable_methods<'a, 'tcx>(
+pub fn vtable_methods<'a, 'tcx>(


This need not be public anymore.

🤦‍ Yep, Another one that I'll do real quick.

theotherjimmy · 2017-10-09T18:07:16Z

@nikomatsakis Thanks for the review. I corrected all of the minor suggestions.

nikomatsakis · 2017-10-09T21:29:27Z

@bors r+

bors · 2017-10-09T21:29:28Z

📌 Commit cb7e101 has been approved by nikomatsakis

bors · 2017-10-11T01:26:28Z

⌛ Testing commit cb7e1016e48ca77277d82d427971c0988ccda497 with merge e66fd14c879d45c57f5c293e922a72de6b666a0d...

bors · 2017-10-11T01:34:13Z

💔 Test failed - status-travis

theotherjimmy · 2017-10-11T01:37:37Z

I don't think this is my fault:

Failed to fetch URL https://dl.google.com/android/repository/addons_list-2.xml, reason: peer not authenticated

kennytm · 2017-10-11T06:38:51Z

@bors retry

Android SDK HTTPS issue (fixed in ci: Fix installing the Android SDK #45193)

bors · 2017-10-12T10:12:10Z

☔ The latest upstream changes (presumably #44967) made this pull request unmergeable. Please resolve the merge conflicts.

aidanhs · 2017-10-12T13:58:45Z

@bors r=nikomatsakis

bors · 2017-10-12T13:58:46Z

📌 Commit 09258e7 has been approved by nikomatsakis

aidanhs · 2017-10-12T15:41:21Z

Wait @bors r-

[00:08:12] �[1m�[31merror�(B�[m�[1m: recursion limit reached while expanding the macro `stringify`�(B�[m
[00:08:12]    �(B�[m�[1m�[34m--> �(B�[m/checkout/src/librustc/ty/maps/plumbing.rs:563:26�(B�[m
[00:08:12]     �(B�[m�[1m�[34m|�(B�[m
[00:08:12] �[1m�[34m563�(B�[m �(B�[m�[1m�[34m| �(B�[m  �(B�[m                         stringify!($name), key);�(B�[m
[00:08:12]     �(B�[m�[1m�[34m| �(B�[m                           �(B�[m�[1m�[31m^^^^^^^^^^^^^^^^^�(B�[m
[00:08:12]     �(B�[m�[1m�[34m| �(B�[m
[00:08:12]    �(B�[m�[1m�[34m::: �(B�[m/checkout/src/librustc/ty/maps/mod.rs�(B�[m
[00:08:12]     �(B�[m�[1m�[34m|�(B�[m
[00:08:12] �[1m�[34m78�(B�[m  �(B�[m�[1m�[34m| �(B�[m�[1m�[34m/�(B�[m �(B�[mdefine_maps! { <'tcx>�(B�[m
[00:08:12] �[1m�[34m79�(B�[m  �(B�[m�[1m�[34m| �(B�[m�[1m�[34m|�(B�[m �(B�[m    /// Records the type of every item.�(B�[m
[00:08:12] �[1m�[34m80�(B�[m  �(B�[m�[1m�[34m| �(B�[m�[1m�[34m|�(B�[m �(B�[m    [] fn type_of: TypeOfItem(DefId) -> Ty<'tcx>,�(B�[m
[00:08:12] �[1m�[34m81�(B�[m  �(B�[m�[1m�[34m| �(B�[m�[1m�[34m|�(B�[m
[00:08:12] �[1m�[34m...�(B�[m   �(B�[m�[1m�[34m|�(B�[m
[00:08:12] �[1m�[34m341�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[34m|�(B�[m �(B�[m    [] fn has_clone_closures: HasCloneClosures(CrateNum) -> bool,�(B�[m
[00:08:12] �[1m�[34m342�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[34m|�(B�[m �(B�[m}�(B�[m
[00:08:12]     �(B�[m�[1m�[34m| �(B�[m�[1m�[34m|_-�(B�[m �(B�[m�[1m�[34min this macro invocation�(B�[m
[00:08:12]     �(B�[m�[1m�[34m|�(B�[m
[00:08:12]     �(B�[m�[1m�[34m= �(B�[m�[1mhelp�(B�[m: consider adding a `#![recursion_limit="512"]` attribute to your crate�(B�[m
[00:08:12] 
[00:08:12] �[m�[m�[31m�[1merror:�[m Could not compile `rustc`.

theotherjimmy · 2017-10-12T16:52:25Z

Well, the good is that I can reproduce the problem. The bad is that I have no idea why it's happening.

theotherjimmy · 2017-10-12T21:33:29Z

It seems like the macro that implements the provider structure, define_provider_struct!, was using some pretty heavy recursion to move data around. I'm working on a direct version of that macro now.

theotherjimmy · 2017-10-12T21:47:33Z

@nikomatsakis @michaelwoerister This change set now includes a big change to plumbing.rs, which should be scrutinized.

nikomatsakis · 2017-10-13T14:13:58Z

src/librustc/ty/maps/plumbing.rs

    (tcx: $tcx:tt,
-     input: (),
-     output: ($(([$name:ident] [$K:ty] [$R:ty]))*)) => {
+     input: ($(([$($modifiers:tt)*] [$name:ident] [$K:ty] [$R:ty]))*)) => {


This change looks fine. It does mean we lose the ability to have modifiers that adjust the type of a query's field. At present, we are not using this capability (we used to be...), so that's probably ok, but we could also just lift the recursion-limit, right?

I'm ok either way, but let's also rebase and remove the merge commit.

theotherjimmy · 2017-10-13T14:32:02Z

Rebasing. Just a moment.

theotherjimmy · 2017-10-13T14:35:22Z

@nikomatsakis rebase complete. Merge commit removed.

nikomatsakis · 2017-10-13T15:09:59Z

@bors r+

bors · 2017-10-13T15:10:00Z

📌 Commit fcb8bc9 has been approved by nikomatsakis

bors · 2017-10-14T06:34:26Z

⌛ Testing commit fcb8bc9 with merge 3037965...

…akis Queryify Vtable methods This query might come with a downside: It converts an iterator to a Vec, which may increase the working set of rustc on programs that use many many traits (I think that's where this is used).

bors · 2017-10-14T08:53:02Z

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

michaelwoerister · 2017-10-16T09:14:26Z

🎉

rust-highfive assigned nikomatsakis Oct 9, 2017

nikomatsakis requested changes Oct 9, 2017

View reviewed changes

theotherjimmy force-pushed the vtable-methods-query branch from 6225f8a to cb7e101 Compare October 9, 2017 18:46

nikomatsakis approved these changes Oct 9, 2017

View reviewed changes

theotherjimmy mentioned this pull request Oct 10, 2017

Extract queries for the trait system operations that are performed in trans #44891

Closed

aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 12, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 13, 2017

nikomatsakis reviewed Oct 13, 2017

View reviewed changes

theotherjimmy added 4 commits October 13, 2017 09:32

Convert return type of get_vtable_methods to Vec

4e116e1

get_vtable_methods => vtable_methods query

b640c2b

Wrap vtable_methods return type in RC

81f9d4e

Remove export of vtable_methods

2160bab

Remove recursion from define_{provider|map}_struct

fcb8bc9

theotherjimmy force-pushed the vtable-methods-query branch from 34cefbd to fcb8bc9 Compare October 13, 2017 14:34

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2017

bors merged commit fcb8bc9 into rust-lang:master Oct 14, 2017

This was referenced Oct 14, 2017

Refactor type memory layouts and ABIs, to be more general and easier to optimize. #45225

Merged

remove or encapsulate the remaining non-query data in tcx #44501

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queryify Vtable methods #45137

Queryify Vtable methods #45137

theotherjimmy commented Oct 9, 2017

rust-highfive commented Oct 9, 2017

nikomatsakis left a comment

nikomatsakis Oct 9, 2017

theotherjimmy Oct 9, 2017

nikomatsakis Oct 9, 2017

theotherjimmy Oct 9, 2017

theotherjimmy commented Oct 9, 2017

nikomatsakis commented Oct 9, 2017

bors commented Oct 9, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

theotherjimmy commented Oct 11, 2017

kennytm commented Oct 11, 2017

bors commented Oct 12, 2017

aidanhs commented Oct 12, 2017

bors commented Oct 12, 2017

aidanhs commented Oct 12, 2017

theotherjimmy commented Oct 12, 2017

theotherjimmy commented Oct 12, 2017 •

edited

Loading

theotherjimmy commented Oct 12, 2017

nikomatsakis Oct 13, 2017

theotherjimmy commented Oct 13, 2017

theotherjimmy commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

bors commented Oct 13, 2017

bors commented Oct 14, 2017

bors commented Oct 14, 2017

michaelwoerister commented Oct 16, 2017

Queryify Vtable methods #45137

Queryify Vtable methods #45137

Conversation

theotherjimmy commented Oct 9, 2017

rust-highfive commented Oct 9, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

theotherjimmy Oct 9, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

theotherjimmy Oct 9, 2017

Choose a reason for hiding this comment

theotherjimmy commented Oct 9, 2017

nikomatsakis commented Oct 9, 2017

bors commented Oct 9, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

theotherjimmy commented Oct 11, 2017

kennytm commented Oct 11, 2017

bors commented Oct 12, 2017

aidanhs commented Oct 12, 2017

bors commented Oct 12, 2017

aidanhs commented Oct 12, 2017

theotherjimmy commented Oct 12, 2017

theotherjimmy commented Oct 12, 2017 • edited Loading

theotherjimmy commented Oct 12, 2017

nikomatsakis Oct 13, 2017

Choose a reason for hiding this comment

theotherjimmy commented Oct 13, 2017

theotherjimmy commented Oct 13, 2017

nikomatsakis commented Oct 13, 2017

bors commented Oct 13, 2017

bors commented Oct 14, 2017

bors commented Oct 14, 2017

michaelwoerister commented Oct 16, 2017

theotherjimmy commented Oct 12, 2017 •

edited

Loading