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 sort_by_cached_key when the key function is not trivial/free #55821

Merged
merged 1 commit into from Nov 30, 2018

Conversation

Projects
None yet
10 participants
@ljedrz
Contributor

ljedrz commented Nov 9, 2018

I'm not 100% sure about def_path_hash (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 9, 2018

r? @cramertj

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

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Nov 15, 2018

@bors try for perf

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned cramertj Nov 15, 2018

@bors

This comment has been minimized.

Contributor

bors commented Nov 15, 2018

⌛️ Trying commit 1649c2e with merge a22afbf...

bors added a commit that referenced this pull request Nov 15, 2018

Auto merge of #55821 - ljedrz:cached_key_sorts, r=<try>
Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.
@eddyb

This comment has been minimized.

Member

eddyb commented Nov 15, 2018

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Nov 15, 2018

@bors

This comment has been minimized.

Contributor

bors commented Nov 15, 2018

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

@@ -585,7 +585,7 @@ fn merge_codegen_units<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>,
// smallest into each other) we're sure to start off with a deterministic
// order (sorted by name). This'll mean that if two cgus have the same size
// the stable sort below will keep everything nice and deterministic.
codegen_units.sort_by_key(|cgu| cgu.name().clone());
codegen_units.sort_by_cached_key(|cgu| cgu.name().clone());

This comment has been minimized.

@sinkuu

sinkuu Nov 16, 2018

Contributor

Can't it be sort_by(|a, b| a.name().cmp(b.name())) to avoid heap allocations completely?
name() returns a ref to InternedString which is Copy, so this clone is free.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 16, 2018

@rust-timer

This comment has been minimized.

rust-timer commented Nov 16, 2018

Success: Queued a22afbf with parent 9649c1f, comparison URL.

@nikomatsakis

Let's see what perf says. I think that the primary_span and def_path_hash calls are likely not a win. Not sure about the others.

@@ -1573,7 +1573,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
.collect();
// ensure that we issue lints in a repeatable order
def_ids.sort_by_key(|&def_id| self.tcx.def_path_hash(def_id));
def_ids.sort_by_cached_key(|&def_id| self.tcx.def_path_hash(def_id));

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 16, 2018

Contributor

As you noted, I think this is already cached -- it's something we do a lot in incremental. You agree, @michaelwoerister ?

This comment has been minimized.

@michaelwoerister

michaelwoerister Nov 21, 2018

Contributor

It's cached, yes, but still not entirely trivial. I.e. it has to do the local/upstream crate dispatch and for upstream DefIds it has to go through a number of function calls:

pub fn def_path_hash(self, def_id: DefId) -> hir_map::DefPathHash {

So, I can imagine this being a win for sorting where each key might be compared against multiple times.

@@ -408,7 +408,7 @@ impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {
.collect::<Vec<_>>();
// existential predicates need to be in a specific order
associated_types.sort_by_key(|item| self.def_path_hash(item.def_id));
associated_types.sort_by_cached_key(|item| self.def_path_hash(item.def_id));

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 16, 2018

Contributor

Also here, of course

@@ -341,7 +341,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
}
if !mbcx.errors_buffer.is_empty() {
mbcx.errors_buffer.sort_by_key(|diag| diag.span.primary_span());
mbcx.errors_buffer.sort_by_cached_key(|diag| diag.span.primary_span());

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 16, 2018

Contributor

This is the 'error reporting' path, so it really doesn't matter... also primary_span is a very simple operation... doesn't strike me as worthwhile.

@@ -985,7 +985,7 @@ fn collect_and_partition_mono_items<'a, 'tcx>(
output.push_str(" @@");
let mut empty = Vec::new();
let cgus = item_to_cgus.get_mut(i).unwrap_or(&mut empty);
cgus.as_mut_slice().sort_by_key(|&(ref name, _)| name.clone());
cgus.as_mut_slice().sort_by_cached_key(|&(ref name, _)| name.clone());

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 16, 2018

Contributor

This might make sense. Not sure what type name has here... :)

@rust-timer

This comment has been minimized.

rust-timer commented Nov 16, 2018

Finished benchmarking try commit a22afbf

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Nov 17, 2018

Looks like a win:

inflate-check
	avg: -2.4%	min: -3.3%	max: 0.3%
keccak-check
	avg: -2.1%	min: -3.1%	max: 0.5%
keccak-opt
	avg: -1.6%	min: -2.7%	max: 0.4%
keccak-debug
	avg: -1.5%	min: -2.5%	max: 0.3%
inflate-debug
	avg: -1.6%	min: -2.3%	max: 0.3%

I can update the commit taking into account the comments - theoretically it could make the wins even nicer.

@ljedrz ljedrz force-pushed the ljedrz:cached_key_sorts branch from 1649c2e to df95268 Nov 17, 2018

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Nov 17, 2018

Not much was left after including the remarks ^^. We might want to run another round of perf to make sure we didn't lose the gains reported by the first run.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 20, 2018

Yeah, we should try again. I'm a bit surprised to see a 3% swing, I have to admit.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 20, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 20, 2018

⌛️ Trying commit df95268 with merge c992107...

bors added a commit that referenced this pull request Nov 20, 2018

Auto merge of #55821 - ljedrz:cached_key_sorts, r=<try>
Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.
@bors

This comment has been minimized.

Contributor

bors commented Nov 20, 2018

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

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Nov 21, 2018

The only change left at this point is in debugging code that is only executed by the test suite -- so I expect no wins there. The original changes (except for the cgu.name().clone()) looked fine to me though.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 26, 2018

@rust-timer

This comment has been minimized.

rust-timer commented Nov 26, 2018

Success: Queued c992107 with parent 3991bfb, comparison URL.

@rust-timer

This comment has been minimized.

rust-timer commented Nov 26, 2018

Finished benchmarking try commit c992107

@ljedrz ljedrz force-pushed the ljedrz:cached_key_sorts branch from df95268 to d4a6e73 Nov 27, 2018

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Nov 27, 2018

Looks like @michaelwoerister was right - the original gains were lost.

I've restored the earlier changes without the Copy (that's free) and primary_span (in error reporting and pretty trivial). This should bring back the greens.

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Nov 27, 2018

lgtm: @bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 27, 2018

📌 Commit d4a6e73 has been approved by michaelwoerister

kennytm added a commit to kennytm/rust that referenced this pull request Nov 27, 2018

Rollup merge of rust-lang#55821 - ljedrz:cached_key_sorts, r=michaelw…
…oerister

Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.

bors added a commit that referenced this pull request Nov 27, 2018

Auto merge of #56276 - kennytm:rollup, r=kennytm
Rollup of 9 pull requests

Successful merges:

 - #55821 (Use sort_by_cached_key when the key function is not trivial/free)
 - #56114 (Enclose type in backticks for "non-exhaustive patterns" error)
 - #56127 (Update an outdated comment in mir building)
 - #56205 (Fix ICE with feature self_struct_ctor)
 - #56216 (Add TryFrom<&[T]> for [T; $N] where T: Copy)
 - #56223 (Make JSON output from -Zprofile-json valid)
 - #56224 (Update cargo)
 - #56236 (Remove unsafe `unsafe` inner function.)
 - #56255 (Update outdated code comments in StringReader)

Failed merges:

r? @ghost

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 28, 2018

Rollup merge of rust-lang#55821 - ljedrz:cached_key_sorts, r=michaelw…
…oerister

Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.

bors added a commit that referenced this pull request Nov 28, 2018

Auto merge of #56325 - pietroalbini:rollup, r=pietroalbini
Rollup of 29 pull requests

Successful merges:

 - #55391 (bootstrap: clean up a few clippy findings)
 - #55821 (Use sort_by_cached_key when the key function is not trivial/free)
 - #56014 (add test for issue #21335)
 - #56021 (avoid features_untracked)
 - #56023 (atomic::Ordering: Get rid of misleading parts of intro)
 - #56044 (Drop partially bound function parameters in the expected order)
 - #56080 (Reduce the amount of bold text at doc.rlo)
 - #56090 (Overhaul `FileSearch` and `SearchPaths`)
 - #56114 (Enclose type in backticks for "non-exhaustive patterns" error)
 - #56124 (Fix small doc mistake on std::io::read::read_to_end)
 - #56127 (Update an outdated comment in mir building)
 - #56131 (Assorted tweaks)
 - #56148 (Add rustc-guide as a submodule)
 - #56149 (Make std::os::unix/linux::fs::MetadataExt::a/m/ctime* documentation clearer)
 - #56165 (drop glue takes in mutable references, it should reflect that in its type)
 - #56205 (Fix ICE with feature self_struct_ctor)
 - #56216 (Add TryFrom<&[T]> for [T; $N] where T: Copy)
 - #56220 (Suggest appropriate place for lifetime when declared after type arguments)
 - #56223 (Make JSON output from -Zprofile-json valid)
 - #56236 (Remove unsafe `unsafe` inner function.)
 - #56245 (Stabilize feature `macro_at_most_once_rep`)
 - #56255 (Update outdated code comments in StringReader)
 - #56257 (rustc-guide has moved to rust-lang/)
 - #56268 (Reuse the `P` in `InvocationCollector::fold_{,opt_}expr`.)
 - #56273 (Add missing doc link)
 - #56285 (move stage0.txt to toplevel directory)
 - #56289 (Fix small typo in comment of thread::stack_size)
 - #56294 (Fix a typo in the documentation of std::ffi)
 - #56300 (Fix alignment of stores to scalar pair)

Failed merges:

r? @ghost

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 29, 2018

Rollup merge of rust-lang#55821 - ljedrz:cached_key_sorts, r=michaelw…
…oerister

Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.

bors added a commit that referenced this pull request Nov 30, 2018

Auto merge of #56353 - pietroalbini:rollup, r=pietroalbini
Rollup of 11 pull requests

Successful merges:

 - #55821 (Use sort_by_cached_key when the key function is not trivial/free)
 - #56014 (add test for issue #21335)
 - #56131 (Assorted tweaks)
 - #56165 (drop glue takes in mutable references, it should reflect that in its type)
 - #56205 (Fix ICE with feature self_struct_ctor)
 - #56216 (Add TryFrom<&[T]> for [T; $N] where T: Copy)
 - #56258 (use top level `fs` functions where appropriate)
 - #56268 (Reuse the `P` in `InvocationCollector::fold_{,opt_}expr`.)
 - #56339 (Remove not used option)
 - #56341 (Rename conversion util; remove duplicate util in librustc_codegen_llvm.)
 - #56349 (rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker)

Failed merges:

 - #56285 (move stage0.txt to toplevel directory)

r? @ghost

kennytm added a commit to kennytm/rust that referenced this pull request Nov 30, 2018

Rollup merge of rust-lang#55821 - ljedrz:cached_key_sorts, r=michaelw…
…oerister

Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.

kennytm added a commit to kennytm/rust that referenced this pull request Nov 30, 2018

Rollup merge of rust-lang#55821 - ljedrz:cached_key_sorts, r=michaelw…
…oerister

Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.

bors added a commit that referenced this pull request Nov 30, 2018

Auto merge of #56376 - kennytm:rollup, r=kennytm
Rollup of 16 pull requests

Successful merges:

 - #55821 (Use sort_by_cached_key when the key function is not trivial/free)
 - #56014 (add test for issue #21335)
 - #56131 (Assorted tweaks)
 - #56216 (Add TryFrom<&[T]> for [T; $N] where T: Copy)
 - #56224 (Update cargo)
 - #56268 (Reuse the `P` in `InvocationCollector::fold_{,opt_}expr`.)
 - #56285 (move stage0.txt to toplevel directory)
 - #56305 (update miri)
 - #56336 (Clean up and streamline the pretty-printer)
 - #56339 (Remove not used option)
 - #56341 (Rename conversion util; remove duplicate util in librustc_codegen_llvm.)
 - #56349 (rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker)
 - #56355 (Add inline attributes and add unit to CommonTypes)
 - #56360 (Optimize local linkchecker program)
 - #56364 (Fix panic with outlives in existential type)
 - #56373 (Update books)

Failed merges:

r? @ghost

kennytm added a commit to kennytm/rust that referenced this pull request Nov 30, 2018

Rollup merge of rust-lang#55821 - ljedrz:cached_key_sorts, r=michaelw…
…oerister

Use sort_by_cached_key when the key function is not trivial/free

I'm not 100% sure about `def_path_hash` (everything it does is inlined) but it seems like a good idea at least for the rest, as they are cloning.

bors added a commit that referenced this pull request Nov 30, 2018

Auto merge of #56381 - kennytm:rollup, r=kennytm
Rollup of 19 pull requests

Successful merges:

 - #55011 (Add libstd Cargo feature "panic_immediate_abort")
 - #55821 (Use sort_by_cached_key when the key function is not trivial/free)
 - #56014 (add test for issue #21335)
 - #56131 (Assorted tweaks)
 - #56214 (Implement chalk unification routines)
 - #56216 (Add TryFrom<&[T]> for [T; $N] where T: Copy)
 - #56268 (Reuse the `P` in `InvocationCollector::fold_{,opt_}expr`.)
 - #56324 (Use raw_entry for more efficient interning)
 - #56336 (Clean up and streamline the pretty-printer)
 - #56337 (Fix const_fn ICE with non-const function pointer)
 - #56339 (Remove not used option)
 - #56341 (Rename conversion util; remove duplicate util in librustc_codegen_llvm.)
 - #56349 (rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker)
 - #56355 (Add inline attributes and add unit to CommonTypes)
 - #56360 (Optimize local linkchecker program)
 - #56364 (Fix panic with outlives in existential type)
 - #56365 (Stabilize self_struct_ctor feature.)
 - #56367 (Moved some feature gate tests to correct location)
 - #56373 (Update books)

@bors bors merged commit d4a6e73 into rust-lang:master Nov 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ljedrz ljedrz deleted the ljedrz:cached_key_sorts branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment