cache symbol names in ty::maps #41507

arielb1 · 2017-04-24T16:39:36Z

this fixes a performance regression introduced in commit 39a58c3.

nikomatsakis · 2017-04-24T16:53:00Z

src/librustc/ty/maps.rs

+#[derive(Clone)]
+pub struct SymbolName {
+    // FIXME: we don't rely on interning or equality here - better have
+    // this be a `&'tcx str`.


What do you mean by "don't rely on interning or equality here"? Maybe you mean for equality? The main thing to be careful of (which &'tcx str would solve) is that we hash the string contents (if you use Symbol, things go awry).

Actually, that comment was needed because a previous try used Symbol there, which as you said is dangerous. I'll remove it.

nikomatsakis · 2017-04-24T16:54:19Z

src/librustc_trans/context.rs

@@ -164,7 +154,8 @@ pub struct LocalCrateContext<'a, 'tcx: 'a> {
    /// A counter that is used for generating local symbol names
    local_gen_sym_counter: Cell<usize>,

-    symbol_cache: &'a SymbolCache<'a, 'tcx>,
+    /// A placeholder so we can add lifetimes
+    placeholder: PhantomData<&'a ()>,


Why not just remove 'a? I think I added it in my PR expressly for this field. But whatever, seems fine.

Missed that. Didn't want to cause unnecessary diffs.

Seems fine.

nikomatsakis · 2017-04-24T16:55:47Z

src/librustc_trans/back/symbol_export.rs

-    symbol_map.get(TransItem::Fn(instance))
-              .map(str::to_owned)
-              .unwrap_or_else(|| symbol_name(instance, tcx))
+    str::to_owned(&symbol_name(instance, tcx))


Shouldn't we just replace this entire function with a call to symbol_name (and change return type to InternedString, of course)? It seems like we should remove the secondary caching performed by SymbolMap, and instead have that also use the queries, no?

nikomatsakis

LGTM, but I'm curious what you think about removing the SymbolMap too.

nikomatsakis · 2017-04-24T17:43:53Z

src/librustc_trans/partitioning.rs

+        let items: Vec<_> = self.items.iter().map(|(&i, &l)| (i, l)).collect();
+        let mut items : Vec<_> = items.iter()
+            .map(|il| (il, item_sort_key(tcx, il.0))).collect();
+        items.sort_by(|&(_, ref key1), &(_, ref key2)| key1.cmp(key2));


I'm confused by this. Aren't there sometimes multiple items with the same node-id that nonetheless deserve a consistent ordering (e.g., different instantiations of the same instance?)

Yup. Let me fix that.

nikomatsakis

Curious about the sorting thing, otherwise seems good.

nikomatsakis · 2017-04-24T17:44:12Z

src/librustc_trans/partitioning.rs

        for cgu in cgus {
            debug!("CodegenUnit {}:", cgu.name);

            for (trans_item, linkage) in &cgu.items {
-                let symbol_name = symbol_cache.get(*trans_item);
+                let symbol_name = trans_item.symbol_name(tcx);


👍 to making this a method, the fn was awkward.

nikomatsakis · 2017-04-24T17:44:31Z

src/librustc_trans/context.rs

@@ -164,7 +154,8 @@ pub struct LocalCrateContext<'a, 'tcx: 'a> {
    /// A counter that is used for generating local symbol names
    local_gen_sym_counter: Cell<usize>,

-    symbol_cache: &'a SymbolCache<'a, 'tcx>,
+    /// A placeholder so we can add lifetimes
+    placeholder: PhantomData<&'a ()>,


Seems fine.

nikomatsakis · 2017-04-24T17:45:37Z

src/librustc_trans/back/symbol_names.rs

+    hasher.finish()
+}
+
+pub fn symbol_name<'a, 'tcx>(instance: Instance<'tcx>, tcx: TyCtxt<'a, 'tcx, 'tcx>)


Total nit, but I'd expect tcx to be the first argument. Having this be a method seems even better (didn't you do this elsewhere? Or is that just for TransItem?) But I don't care much.

It was this way originally. Let me move it all the way to a method on Instance.

nikomatsakis · 2017-04-24T17:56:58Z

src/librustc_trans/partitioning.rs

-            } else {
-                ItemSortKey::Remote(item.symbol_name(tcx))
-            }
+            }, item.symbol_name(tcx))


👍 simpler this way anyhow

arielb1 · 2017-04-24T18:13:17Z

@bors r=nikomatsakis

bors · 2017-04-24T18:13:17Z

📌 Commit af63f14 has been approved by nikomatsakis

arielb1 · 2017-04-24T18:19:06Z

Fixed finish micro-opt

@bors r=nikomatsakis

bors · 2017-04-24T18:19:07Z

📌 Commit 07d1233 has been approved by nikomatsakis

bors · 2017-04-26T12:36:09Z

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

this fixes a performance regression introduced in commit 39a58c3.

arielb1 · 2017-04-26T14:57:27Z

@bors r=nikomatsakis

bors · 2017-04-26T14:57:27Z

📌 Commit 07b16cb has been approved by nikomatsakis

bors · 2017-04-27T06:32:02Z

⌛ Testing commit 07b16cb with merge aed35b2...

bors · 2017-04-27T07:50:39Z

💔 Test failed - status-appveyor

aidanhs · 2017-04-27T09:01:13Z

@bors retry

Appveyor network issues https://appveyor.statuspage.io/incidents/06gzq846jl9x

bors · 2017-04-27T13:18:01Z

⌛ Testing commit 07b16cb with merge 4961d72...

@nikomatsakis

cache symbol names in ty::maps this fixes a performance regression introduced in commit 39a58c3. r? @nikomatsakis

bors · 2017-04-27T16:38:05Z

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

rust-highfive assigned nikomatsakis Apr 24, 2017

nikomatsakis reviewed Apr 24, 2017

View reviewed changes

nikomatsakis approved these changes Apr 24, 2017

View reviewed changes

arielb1 force-pushed the symbol-cache branch from 90860a5 to 7159141 Compare April 24, 2017 17:35

nikomatsakis reviewed Apr 24, 2017

View reviewed changes

arielb1 force-pushed the symbol-cache branch 3 times, most recently from a8c9623 to 1199e3e Compare April 24, 2017 17:50

nikomatsakis approved these changes Apr 24, 2017

View reviewed changes

arielb1 force-pushed the symbol-cache branch from 1199e3e to af63f14 Compare April 24, 2017 18:12

arielb1 force-pushed the symbol-cache branch from af63f14 to 07d1233 Compare April 24, 2017 18:18

frewsxcv mentioned this pull request Apr 24, 2017

Rollup of 6 pull requests #41516

Closed

arielb1 added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 25, 2017

arielb1 added 3 commits April 26, 2017 17:45

cache symbol names in ty::maps

a517343

this fixes a performance regression introduced in commit 39a58c3.

refactor away trans::symbol_map

9aac5fc

don't allocate in get_symbol_hash

07b16cb

arielb1 force-pushed the symbol-cache branch from 07d1233 to 07b16cb Compare April 26, 2017 14:57

frewsxcv mentioned this pull request Apr 27, 2017

Rollup of 4 pull requests #41573

Closed

bors added a commit that referenced this pull request Apr 27, 2017

Auto merge of #41507 - arielb1:symbol-cache, r=nikomatsakis

4961d72

cache symbol names in ty::maps this fixes a performance regression introduced in commit 39a58c3. r? @nikomatsakis

bors merged commit 07b16cb into rust-lang:master Apr 27, 2017

This was referenced Apr 27, 2017

query for describe_def #41534

Merged

Rollup of 10 pull requests #41579

Closed

make *most* maps private #41368

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache symbol names in ty::maps #41507

cache symbol names in ty::maps #41507

arielb1 commented Apr 24, 2017 •

edited by eddyb

Loading

nikomatsakis Apr 24, 2017

arielb1 Apr 24, 2017

nikomatsakis Apr 24, 2017

arielb1 Apr 24, 2017

nikomatsakis Apr 24, 2017

nikomatsakis Apr 24, 2017

nikomatsakis left a comment

nikomatsakis Apr 24, 2017

arielb1 Apr 24, 2017

nikomatsakis left a comment

nikomatsakis Apr 24, 2017

nikomatsakis Apr 24, 2017

nikomatsakis Apr 24, 2017

arielb1 Apr 24, 2017

nikomatsakis Apr 24, 2017

arielb1 commented Apr 24, 2017

bors commented Apr 24, 2017

arielb1 commented Apr 24, 2017

bors commented Apr 24, 2017

bors commented Apr 26, 2017

arielb1 commented Apr 26, 2017

bors commented Apr 26, 2017

bors commented Apr 27, 2017

bors commented Apr 27, 2017

aidanhs commented Apr 27, 2017

bors commented Apr 27, 2017

bors commented Apr 27, 2017

cache symbol names in ty::maps #41507

cache symbol names in ty::maps #41507

Conversation

arielb1 commented Apr 24, 2017 • edited by eddyb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Apr 24, 2017

bors commented Apr 24, 2017

arielb1 commented Apr 24, 2017

bors commented Apr 24, 2017

bors commented Apr 26, 2017

arielb1 commented Apr 26, 2017

bors commented Apr 26, 2017

bors commented Apr 27, 2017

bors commented Apr 27, 2017

aidanhs commented Apr 27, 2017

bors commented Apr 27, 2017

bors commented Apr 27, 2017

arielb1 commented Apr 24, 2017 •

edited by eddyb

Loading