Simplify internals of {Rc,Arc}::default#152591
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
da1ddcf to
ba5ff3d
Compare
| @@ -30,8 +31,7 @@ pub fn new_uninit(x: u64) -> Arc<[u64; 1000]> { | |||
| // CHECK-LABEL: @new_uninit_slice | |||
| #[no_mangle] | |||
| pub fn new_uninit_slice(x: u64) -> Arc<[u64]> { | |||
| // CHECK: call alloc::sync::arcinner_layout_for_value_layout | |||
| // CHECK-NOT: call alloc::sync::arcinner_layout_for_value_layout | |||
| // CHECK-NOT: %[[B:.+]] = alloca | |||
There was a problem hiding this comment.
The breadcrumbs for this codegen test points back at #111634 which had to do with internal refactorings about how methods were done. I don't believe that these codegen tests are really all that applicable any more so I did my best to at least keep them somewhat, but the extra #[inline] in this PR effectively means that the internals this test relies on also changed which means that the test needed an update one way or another.
This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in 131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change. The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
ba5ff3d to
ce4c17f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Good idea! |
…lt, r=joboet
Simplify internals of `{Rc,Arc}::default`
This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in rust-lang#131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change.
The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
Rollup of 14 pull requests Successful merges: - #150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - #151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - #151740 (Fix short backtraces from stripped executables) - #151871 (don't use env with infer vars) - #152591 (Simplify internals of `{Rc,Arc}::default`) - #152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - #152908 (Enable rust.remap-debuginfo in the dist profile) - #152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - #152767 (fix typo in `carryless_mul` macro invocation) - #152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - #152871 (Fix warnings in rs{begin,end}.rs files) - #152921 (Add build.rustdoc option to bootstrap config) - #152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - #152937 (remove unneeded reboxing)
…lt, r=joboet
Simplify internals of `{Rc,Arc}::default`
This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in rust-lang#131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change.
The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
Rollup of 15 pull requests Successful merges: - #149366 (GVN: consider constants of primitive types as deterministic) - #150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - #151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - #151871 (don't use env with infer vars) - #152591 (Simplify internals of `{Rc,Arc}::default`) - #152657 (std: move `exit` out of PAL) - #152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - #152908 (Enable rust.remap-debuginfo in the dist profile) - #152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - #152767 (fix typo in `carryless_mul` macro invocation) - #152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - #152871 (Fix warnings in rs{begin,end}.rs files) - #152921 (Add build.rustdoc option to bootstrap config) - #152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - #152937 (remove unneeded reboxing)
…lt, r=joboet
Simplify internals of `{Rc,Arc}::default`
This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in rust-lang#131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change.
The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
Rollup of 14 pull requests Successful merges: - #150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - #151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - #151871 (don't use env with infer vars) - #152591 (Simplify internals of `{Rc,Arc}::default`) - #152657 (std: move `exit` out of PAL) - #152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - #152908 (Enable rust.remap-debuginfo in the dist profile) - #152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - #152767 (fix typo in `carryless_mul` macro invocation) - #152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - #152871 (Fix warnings in rs{begin,end}.rs files) - #152921 (Add build.rustdoc option to bootstrap config) - #152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - #152937 (remove unneeded reboxing)
…lt, r=joboet
Simplify internals of `{Rc,Arc}::default`
This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in rust-lang#131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change.
The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
Rollup of 13 pull requests Successful merges: - #150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - #151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - #151871 (don't use env with infer vars) - #152591 (Simplify internals of `{Rc,Arc}::default`) - #152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - #152908 (Enable rust.remap-debuginfo in the dist profile) - #152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - #152767 (fix typo in `carryless_mul` macro invocation) - #152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - #152871 (Fix warnings in rs{begin,end}.rs files) - #152921 (Add build.rustdoc option to bootstrap config) - #152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - #152937 (remove unneeded reboxing)
…lt, r=joboet
Simplify internals of `{Rc,Arc}::default`
This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in rust-lang#131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change.
The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
Rollup of 15 pull requests Successful merges: - #150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - #151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - #151871 (don't use env with infer vars) - #152591 (Simplify internals of `{Rc,Arc}::default`) - #152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - #152908 (Enable rust.remap-debuginfo in the dist profile) - #147859 (reduce the amount of panics in `{TokenStream, Literal}::from_str` calls) - #152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - #152767 (fix typo in `carryless_mul` macro invocation) - #152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - #152871 (Fix warnings in rs{begin,end}.rs files) - #152879 (Remove `impl IntoQueryParam<P> for &'a P`.) - #152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - #152937 (remove unneeded reboxing) - #152953 (Fix typo in armv7a-vex-v5.md)
…lt, r=joboet
Simplify internals of `{Rc,Arc}::default`
This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in rust-lang#131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change.
The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
…uwer Rollup of 14 pull requests Successful merges: - #150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - #151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - #151871 (don't use env with infer vars) - #152591 (Simplify internals of `{Rc,Arc}::default`) - #152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - #147859 (reduce the amount of panics in `{TokenStream, Literal}::from_str` calls) - #152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - #152767 (fix typo in `carryless_mul` macro invocation) - #152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - #152871 (Fix warnings in rs{begin,end}.rs files) - #152879 (Remove `impl IntoQueryParam<P> for &'a P`.) - #152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - #152937 (remove unneeded reboxing) - #152953 (Fix typo in armv7a-vex-v5.md)
…uwer Rollup of 14 pull requests Successful merges: - #150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - #151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - #151871 (don't use env with infer vars) - #152591 (Simplify internals of `{Rc,Arc}::default`) - #152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - #147859 (reduce the amount of panics in `{TokenStream, Literal}::from_str` calls) - #152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - #152767 (fix typo in `carryless_mul` macro invocation) - #152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - #152871 (Fix warnings in rs{begin,end}.rs files) - #152879 (Remove `impl IntoQueryParam<P> for &'a P`.) - #152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - #152937 (remove unneeded reboxing) - #152953 (Fix typo in armv7a-vex-v5.md)
Rollup merge of #152591 - alexcrichton:simplify-rc-arc-default, r=joboet Simplify internals of `{Rc,Arc}::default` This commit simplifies the internal implementation of `Default` for these two pointer types to have the same performance characteristics as before (a side effect of changes in #131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as `#[inline]` as well, but otherwise the same IR is produced before/after this change. The motivation of this commit is I was studying up on the state of initialization of `Arc` and `Rc` and figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - rust-lang/rust#151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - rust-lang/rust#151871 (don't use env with infer vars) - rust-lang/rust#152591 (Simplify internals of `{Rc,Arc}::default`) - rust-lang/rust#152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - rust-lang/rust#147859 (reduce the amount of panics in `{TokenStream, Literal}::from_str` calls) - rust-lang/rust#152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - rust-lang/rust#152767 (fix typo in `carryless_mul` macro invocation) - rust-lang/rust#152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - rust-lang/rust#152871 (Fix warnings in rs{begin,end}.rs files) - rust-lang/rust#152879 (Remove `impl IntoQueryParam<P> for &'a P`.) - rust-lang/rust#152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - rust-lang/rust#152937 (remove unneeded reboxing) - rust-lang/rust#152953 (Fix typo in armv7a-vex-v5.md)
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#150468 (rustc_target: callconv: powerpc64: Use the ABI set in target options instead of guessing) - rust-lang/rust#151628 (Fix ICE in const eval of packed SIMD types with non-power-of-two element counts) - rust-lang/rust#151871 (don't use env with infer vars) - rust-lang/rust#152591 (Simplify internals of `{Rc,Arc}::default`) - rust-lang/rust#152865 (Fixed ByteStr not padding within its Display trait when no specific alignment is mentioned) - rust-lang/rust#147859 (reduce the amount of panics in `{TokenStream, Literal}::from_str` calls) - rust-lang/rust#152705 (Test(lib/win/proc): Skip `raw_attributes` doctest under Win7) - rust-lang/rust#152767 (fix typo in `carryless_mul` macro invocation) - rust-lang/rust#152837 (fix(codegen): Use `body_codegen_attrs` For Caller In `adjust_target_feature_sig`) - rust-lang/rust#152871 (Fix warnings in rs{begin,end}.rs files) - rust-lang/rust#152879 (Remove `impl IntoQueryParam<P> for &'a P`.) - rust-lang/rust#152933 (Start migration for `LintDiagnostic` items by adding API and migrating `LinkerOutput` lint) - rust-lang/rust#152937 (remove unneeded reboxing) - rust-lang/rust#152953 (Fix typo in armv7a-vex-v5.md)
|
@rust-timer build 367f121 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (367f121): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 481.835s -> 480.386s (-0.30%) |
|
This caused the perf regression in #152965 |
|
Is this at the threshold that I should proactively revert this pending further investigation? Or should I be the one digging in to see what the regression stems from? Given how minor this is I'd probably just revert and abandon the change in that case, but I wanted to be sure to clarify if the responsibility for addressing this lies with me. |
|
The regression is not very large but large enough that if there is no benefit to this PR other than simpler code I'd propose to revert it |
Revert "Simplify internals of `{Rc,Arc}::default`"
This reverts rust-lang#152591 following a [perf run being done](rust-lang#152591 (comment)). I'm not really positioned at this time to dive in further and understand the performance regressions, so I'll back away from `Rc`/`Arc` and leave them as-is.
Revert "Simplify internals of `{Rc,Arc}::default`"
This reverts #152591 following a [perf run being done](#152591 (comment)). I'm not really positioned at this time to dive in further and understand the performance regressions, so I'll back away from `Rc`/`Arc` and leave them as-is.
This commit simplifies the internal implementation of
Defaultfor these two pointer types to have the same performance characteristics as before (a side effect of changes in #131460) while avoid use of internal private APIs of Rc/Arc. To preserve the same codegen as before some non-generic functions needed to be tagged as#[inline]as well, but otherwise the same IR is produced before/after this change.The motivation of this commit is I was studying up on the state of initialization of
ArcandRcand figured it'd be nicer to reduce the use of internal APIs and instead use public stable APIs where possible, even in the implementation itself.