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

Add a "cheap" mode for `compute_missing_ctors`. #55167

Merged
merged 1 commit into from Oct 26, 2018

Conversation

Projects
None yet
4 participants
@nnethercote
Contributor

nnethercote commented Oct 18, 2018

compute_missing_ctors produces a vector. It is called a lot, but the
vector is almost always only checked for emptiness.

This commit introduces a specialized variant of compute_missing_ctors
(called is_missing_ctors_empty) that determines if the resulting set
would be empty, and changes the callsite so that compute_missing_ctors
is only called in the rare cases where it is needed. The code
duplication is unfortunate but I can't see a better way to do it.

This change reduces instruction counts for several benchmarks up to 2%.

r? @varkor

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Oct 18, 2018

Some local benchmarking results:

helloworld-check
        avg: -1.9%      min: -2.0%      max: -1.7%
ctfe-stress-check
        avg: -0.9%?     min: -1.9%?     max: 0.5%?
unify-linearly-check
        avg: -1.3%      min: -1.8%      max: -0.9%
deeply-nested-check
        avg: -1.1%      min: -1.7%      max: -0.9%
issue-46449-check
        avg: -1.2%      min: -1.4%      max: -0.8%
coercions-check
        avg: 0.0%?      min: -1.0%?     max: 1.0%?
style-servo-check
        avg: -0.5%      min: -1.0%      max: 0.0%
@varkor

This comment has been minimized.

Member

varkor commented Oct 18, 2018

This is a nice performance win, but I'm hesitant about just duplicating the logic. What do you think about changing the return type of compute_missing_ctors to:

enum MissingCtors<'tcx> {
	Empty,
	Nonempty,
	NonemptyWithCtors(Vec<Constructor<'tcx>>),
}

and then passing an extra parameter to compute_missing_ctors that determines whether to construct the vector, or to just use Nonempty?

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Oct 18, 2018

Ok, I can do that tomorrow.

@nnethercote nnethercote changed the title from Add `is_missing_ctors_empty`. to Add a "cheap" mode for `compute_missing_ctors`. Oct 19, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Oct 19, 2018

@varkor: I've updated the code as requested.

// Return a set of constructors equivalent to `all_ctors \ used_ctors`.
// Used by `compute_missing_ctors`.
#[derive(PartialEq)]
enum Cost {

This comment has been minimized.

@varkor

varkor Oct 19, 2018

Member

I feel naming the variants of the function in terms of cost, rather than behaviour, is unhelpful — you can't guess what difference in behaviour might result from providing Cheap rather than Expensive (even if that is the motivation for the change). I'd rather stick with the comment about performance above compute_missing_ctors and give the variants more descriptive names.

@varkor

This comment has been minimized.

Member

varkor commented Oct 19, 2018

r=me after the comment about naming. Thanks for investigating this!

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Oct 19, 2018

What names do you suggest instead of Cost, Cheap, Expensive, and the new parameter to missing_costs?

@varkor

This comment has been minimized.

Member

varkor commented Oct 22, 2018

(Sorry, GitHub's been preventing me from commenting.)

Perhaps something like the following for the input, with #55167 (comment) as the output.

/// A request for missing constructor data in terms of either:
///	(a) Simply whether there are any missing constructors.
///	(b) Exactly which constructors are missing.
/// This is to improve performance in cases where the full constructor information is unnecessary.
enum MissingCtorsInfo {
	/// Corresponds to `MissingCtors::Empty` and `MissingCtors::Nonempty`.
	Emptiness,
	/// Corresponds to `MissingCtors::NonemptyWithCtors`.
	Ctors,
}
@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Oct 22, 2018

Ok, but I'll change NonemptyWithCtors, because it can be empty.

Add a cheap mode for `compute_missing_ctors`.
`compute_missing_ctors` is called a lot. It produces a vector, which can
be reasonably large (e.g. 100+ elements), but the vector is almost
always only checked for emptiness.

This commit changes `compute_missing_ctors` so it can be called in a
cheap way that just indicates if the vector would be empty. If
necessary, the function can subsequently be called in an expensive way
to compute the full vector.

This change reduces instruction counts for several benchmarks up to 2%.
@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Oct 22, 2018

Updated as requested.

@varkor

This comment has been minimized.

Member

varkor commented Oct 22, 2018

Thanks for looking into this! @bors r+

@bors

This comment has been minimized.

Contributor

bors commented Oct 22, 2018

📌 Commit b5336c0 has been approved by varkor

kennytm added a commit to kennytm/rust that referenced this pull request Oct 24, 2018

Rollup merge of rust-lang#55167 - nnethercote:is_missing_ctors_empty,…
… r=varkor

Add a "cheap" mode for `compute_missing_ctors`.

`compute_missing_ctors` produces a vector. It is called a lot, but the
vector is almost always only checked for emptiness.

This commit introduces a specialized variant of `compute_missing_ctors`
(called `is_missing_ctors_empty`) that determines if the resulting set
would be empty, and changes the callsite so that `compute_missing_ctors`
is only called in the rare cases where it is needed. The code
duplication is unfortunate but I can't see a better way to do it.

This change reduces instruction counts for several benchmarks up to 2%.

r? @varkor

bors added a commit that referenced this pull request Oct 25, 2018

Auto merge of #55320 - kennytm:rollup, r=kennytm
Rollup of 22 pull requests

Successful merges:

 - #53507 (Add doc for impl From for Waker)
 - #54626 (rustc: Tweak filenames encoded into metadata)
 - #54965 (update tcp stream documentation)
 - #54977 (Accept `Option<Box<$t:ty>>` in macro argument)
 - #55138 (in which unused-parens suggestions heed what the user actually wrote)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55173 (Suggest appropriate syntax on missing lifetime specifier in return type)
 - #55225 (Move cg_llvm:🔙:linker to cg_utils)
 - #55245 (submodules: update clippy from 5afdf8b7 to b1d03437)
 - #55247 (Clarified code example in char primitive doc)
 - #55251 (Fix a typo in the documentation of RangeInclusive)
 - #55253 (only issue "variant of the expected type" suggestion for enums)
 - #55254 (Correct trailing ellipsis in name_from_pat)
 - #55257 (Allow extern statics with an extern type)
 - #55258 (Fix Rustdoc ICE when checking blanket impls)
 - #55262 (Change the ICE from #55223 to a hard error)
 - #55269 (fix typos in various places)
 - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators)
 - #55282 (Remove redundant clone)
 - #55285 (Do some copy editing on the release notes)
 - #55291 (Update stdsimd submodule)
 - #55303 (Update compiler-builtins submodule)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Oct 25, 2018

Auto merge of #55320 - kennytm:rollup, r=kennytm
Rollup of 28 pull requests

Successful merges:

 - #53507 (Add doc for impl From for Waker)
 - #53931 (Gradually expanding libstd's keyword documentation)
 - #54626 (rustc: Tweak filenames encoded into metadata)
 - #54921 (Add line numbers option to rustdoc)
 - #54965 (update tcp stream documentation)
 - #54977 (Accept `Option<Box<$t:ty>>` in macro argument)
 - #55138 (in which unused-parens suggestions heed what the user actually wrote)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55173 (Suggest appropriate syntax on missing lifetime specifier in return type)
 - #55200 (Documents `From` implementations for `Stdio`)
 - #55225 (Move cg_llvm:🔙:linker to cg_utils)
 - #55245 (submodules: update clippy from 5afdf8b7 to b1d03437)
 - #55247 (Clarified code example in char primitive doc)
 - #55251 (Fix a typo in the documentation of RangeInclusive)
 - #55253 (only issue "variant of the expected type" suggestion for enums)
 - #55254 (Correct trailing ellipsis in name_from_pat)
 - #55257 (Allow extern statics with an extern type)
 - #55258 (Fix Rustdoc ICE when checking blanket impls)
 - #55262 (Change the ICE from #55223 to a hard error)
 - #55269 (fix typos in various places)
 - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators)
 - #55282 (Remove redundant clone)
 - #55285 (Do some copy editing on the release notes)
 - #55291 (Update stdsimd submodule)
 - #55296 (Set RUST_BACKTRACE=0 for rustdoc-ui/failed-doctest-output.rs)
 - #55303 (Update compiler-builtins submodule)
 - #55306 (Regression test for #54478.)
 - #55328 (Fix doc for new copysign functions)

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 25, 2018

Rollup merge of rust-lang#55167 - nnethercote:is_missing_ctors_empty,…
… r=varkor

Add a "cheap" mode for `compute_missing_ctors`.

`compute_missing_ctors` produces a vector. It is called a lot, but the
vector is almost always only checked for emptiness.

This commit introduces a specialized variant of `compute_missing_ctors`
(called `is_missing_ctors_empty`) that determines if the resulting set
would be empty, and changes the callsite so that `compute_missing_ctors`
is only called in the rare cases where it is needed. The code
duplication is unfortunate but I can't see a better way to do it.

This change reduces instruction counts for several benchmarks up to 2%.

r? @varkor

bors added a commit that referenced this pull request Oct 25, 2018

Auto merge of #55357 - pietroalbini:rollup, r=pietroalbini
Rollup of 15 pull requests

Successful merges:

 - #54490 (Rewrite the `UnconditionalRecursion` lint to use MIR)
 - #54626 (rustc: Tweak filenames encoded into metadata)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #54965 (update tcp stream documentation)
 - #55010 (Add template parameter debuginfo to generic types)
 - #55150 (Do not allow moving out of thread local under ast borrowck)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55221 (Don't emit cannot move errors twice in migrate mode)
 - #55238 (Remove the `alloc_jemalloc` crate)
 - #55244 (Don't rerun MIR passes when inlining)
 - #55257 (Allow extern statics with an extern type)
 - #55269 (fix typos in various places)
 - #55301 (List allowed tokens after macro fragments)
 - #55325 (Fix link to macros chapter)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Oct 25, 2018

Auto merge of #55357 - pietroalbini:rollup, r=pietroalbini
Rollup of 15 pull requests

Successful merges:

 - #54490 (Rewrite the `UnconditionalRecursion` lint to use MIR)
 - #54626 (rustc: Tweak filenames encoded into metadata)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #54965 (update tcp stream documentation)
 - #55010 (Add template parameter debuginfo to generic types)
 - #55150 (Do not allow moving out of thread local under ast borrowck)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55221 (Don't emit cannot move errors twice in migrate mode)
 - #55238 (Remove the `alloc_jemalloc` crate)
 - #55244 (Don't rerun MIR passes when inlining)
 - #55257 (Allow extern statics with an extern type)
 - #55269 (fix typos in various places)
 - #55301 (List allowed tokens after macro fragments)
 - #55325 (Fix link to macros chapter)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Oct 25, 2018

Auto merge of #55357 - pietroalbini:rollup, r=pietroalbini
Rollup of 15 pull requests

Successful merges:

 - #54490 (Rewrite the `UnconditionalRecursion` lint to use MIR)
 - #54626 (rustc: Tweak filenames encoded into metadata)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #54965 (update tcp stream documentation)
 - #55010 (Add template parameter debuginfo to generic types)
 - #55150 (Do not allow moving out of thread local under ast borrowck)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55221 (Don't emit cannot move errors twice in migrate mode)
 - #55238 (Remove the `alloc_jemalloc` crate)
 - #55244 (Don't rerun MIR passes when inlining)
 - #55257 (Allow extern statics with an extern type)
 - #55269 (fix typos in various places)
 - #55301 (List allowed tokens after macro fragments)
 - #55325 (Fix link to macros chapter)

Failed merges:

r? @ghost

kennytm added a commit to kennytm/rust that referenced this pull request Oct 26, 2018

Rollup merge of rust-lang#55167 - nnethercote:is_missing_ctors_empty,…
… r=varkor

Add a "cheap" mode for `compute_missing_ctors`.

`compute_missing_ctors` produces a vector. It is called a lot, but the
vector is almost always only checked for emptiness.

This commit introduces a specialized variant of `compute_missing_ctors`
(called `is_missing_ctors_empty`) that determines if the resulting set
would be empty, and changes the callsite so that `compute_missing_ctors`
is only called in the rare cases where it is needed. The code
duplication is unfortunate but I can't see a better way to do it.

This change reduces instruction counts for several benchmarks up to 2%.

r? @varkor

bors added a commit that referenced this pull request Oct 26, 2018

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

Successful merges:

 - #53996 ([CI] Run a `thumbv7m-none-eabi` binary using `qemu-system-arm` [IRR-2018-embedded])
 - #54816 (Don't try to promote already promoted out temporaries)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55258 (Fix Rustdoc ICE when checking blanket impls)
 - #55262 (Change the ICE from #55223 to a hard error)
 - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators)
 - #55292 (Macro diagnostics tweaks)
 - #55298 (Point at macro definition when no rules expect token)
 - #55301 (List allowed tokens after macro fragments)
 - #55325 (Fix link to macros chapter)
 - #55343 (rustbuild: fix remap-debuginfo when building a release)
 - #55346 (Shrink `Statement`.)
 - #55358 (Remove redundant clone (2))
 - #55363 (Bump cargo-vendor version to 0.1.17)

bors added a commit that referenced this pull request Oct 26, 2018

Auto merge of #55371 - kennytm:rollup, r=kennytm
Rollup of 15 pull requests

Successful merges:

 - #53996 ([CI] Run a `thumbv7m-none-eabi` binary using `qemu-system-arm` [IRR-2018-embedded])
 - #54816 (Don't try to promote already promoted out temporaries)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55258 (Fix Rustdoc ICE when checking blanket impls)
 - #55262 (Change the ICE from #55223 to a hard error)
 - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators)
 - #55292 (Macro diagnostics tweaks)
 - #55298 (Point at macro definition when no rules expect token)
 - #55301 (List allowed tokens after macro fragments)
 - #55325 (Fix link to macros chapter)
 - #55343 (rustbuild: fix remap-debuginfo when building a release)
 - #55346 (Shrink `Statement`.)
 - #55358 (Remove redundant clone (2))

kennytm added a commit to kennytm/rust that referenced this pull request Oct 26, 2018

Rollup merge of rust-lang#55167 - nnethercote:is_missing_ctors_empty,…
… r=varkor

Add a "cheap" mode for `compute_missing_ctors`.

`compute_missing_ctors` produces a vector. It is called a lot, but the
vector is almost always only checked for emptiness.

This commit introduces a specialized variant of `compute_missing_ctors`
(called `is_missing_ctors_empty`) that determines if the resulting set
would be empty, and changes the callsite so that `compute_missing_ctors`
is only called in the rare cases where it is needed. The code
duplication is unfortunate but I can't see a better way to do it.

This change reduces instruction counts for several benchmarks up to 2%.

r? @varkor

bors added a commit that referenced this pull request Oct 26, 2018

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

Successful merges:

 - #54816 (Don't try to promote already promoted out temporaries)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55258 (Fix Rustdoc ICE when checking blanket impls)
 - #55262 (Change the ICE from #55223 to a hard error)
 - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators)
 - #55292 (Macro diagnostics tweaks)
 - #55298 (Point at macro definition when no rules expect token)
 - #55301 (List allowed tokens after macro fragments)
 - #55302 (Extend the impl_stable_hash_for! macro for miri.)
 - #55325 (Fix link to macros chapter)
 - #55343 (rustbuild: fix remap-debuginfo when building a release)
 - #55346 (Shrink `Statement`.)
 - #55358 (Remove redundant clone (2))
 - #55363 (Bump cargo-vendor version to 0.1.17)
 - #55370 (Update mailmap for estebank)
 - #55375 (Typo fixes in configure_cmake comments)
 - #55378 (rustbuild: use configured linker to build boostrap)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Oct 26, 2018

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

Successful merges:

 - #54816 (Don't try to promote already promoted out temporaries)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55258 (Fix Rustdoc ICE when checking blanket impls)
 - #55262 (Change the ICE from #55223 to a hard error)
 - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators)
 - #55292 (Macro diagnostics tweaks)
 - #55298 (Point at macro definition when no rules expect token)
 - #55301 (List allowed tokens after macro fragments)
 - #55302 (Extend the impl_stable_hash_for! macro for miri.)
 - #55325 (Fix link to macros chapter)
 - #55343 (rustbuild: fix remap-debuginfo when building a release)
 - #55346 (Shrink `Statement`.)
 - #55358 (Remove redundant clone (2))
 - #55370 (Update mailmap for estebank)
 - #55375 (Typo fixes in configure_cmake comments)
 - #55378 (rustbuild: use configured linker to build boostrap)
 - #55379 (validity: assert that unions are non-empty)

bors added a commit that referenced this pull request Oct 26, 2018

Auto merge of #55382 - kennytm:rollup, r=kennytm
Rollup of 21 pull requests

Successful merges:

 - #54816 (Don't try to promote already promoted out temporaries)
 - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`)
 - #54921 (Add line numbers option to rustdoc)
 - #55167 (Add a "cheap" mode for `compute_missing_ctors`.)
 - #55258 (Fix Rustdoc ICE when checking blanket impls)
 - #55264 (Compile the libstd we distribute with -Ccodegen-unit=1)
 - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators)
 - #55292 (Macro diagnostics tweaks)
 - #55298 (Point at macro definition when no rules expect token)
 - #55301 (List allowed tokens after macro fragments)
 - #55302 (Extend the impl_stable_hash_for! macro for miri.)
 - #55325 (Fix link to macros chapter)
 - #55343 (rustbuild: fix remap-debuginfo when building a release)
 - #55346 (Shrink `Statement`.)
 - #55358 (Remove redundant clone (2))
 - #55370 (Update mailmap for estebank)
 - #55375 (Typo fixes in configure_cmake comments)
 - #55378 (rustbuild: use configured linker to build boostrap)
 - #55379 (validity: assert that unions are non-empty)
 - #55383 (Use `SmallVec` for the queue in `coerce_unsized`.)
 - #55391 (bootstrap: clean up a few clippy findings)

@bors bors merged commit b5336c0 into rust-lang:master Oct 26, 2018

1 check passed

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