Small optimizations in macro derive

[JulianKnodt]

Some small memory optimizations in macro derives, removing some allocations.

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[davidtwco]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

[bors]

⌛ Trying commit 0e41d272039134a34d06081c4cb9536d646ec019 with merge daac1498047258893dc86ee7a1f908737a5cbb90...

[davidtwco]

Implementation LGTM, queued a perf run, r=me if there's an improvement.

[bors]

☀️ Try build successful - checks-actions
Build commit: daac1498047258893dc86ee7a1f908737a5cbb90 (daac1498047258893dc86ee7a1f908737a5cbb90)

[rust-timer]

Queued daac1498047258893dc86ee7a1f908737a5cbb90 with parent 463ce40, future comparison URL.

@rustbot label: +S-waiting-on-perf

[rust-timer]

Finished benchmarking try commit (daac1498047258893dc86ee7a1f908737a5cbb90): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[JulianKnodt]

I believe this can only be chocked up to noise; if it's alright I'd like to continue working on this

[JulianKnodt]

This should be good, hopefully it has a minor effect?

[davidtwco]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

[bors]

⌛ Trying commit 35299bc07437135d217c19074d62693c74db6caa with merge 43e292cf8756d483a054508a0149e128ec285434...

[bors]

☀️ Try build successful - checks-actions
Build commit: 43e292cf8756d483a054508a0149e128ec285434 (43e292cf8756d483a054508a0149e128ec285434)

[rust-timer]

Queued 43e292cf8756d483a054508a0149e128ec285434 with parent 353f3a3, future comparison URL.

@rustbot label: +S-waiting-on-perf

[rust-timer]

Finished benchmarking try commit (43e292cf8756d483a054508a0149e128ec285434): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[JulianKnodt]

Hm, I can't make heads or tails of whether this is just noise at this point(which probably means it is noise), but let me know what you think.

[davidtwco]

Hm, I can't make heads or tails of whether this is just noise at this point(which probably means it is noise), but let me know what you think.

I agree, it's hard to say if there's any improvement here. If you have other things that you want to try then feel free to, but we might just need to close this unfortunately.

[JulianKnodt]

I'll keep working on it, but will try to avoid perf runs for a bit

[JulianKnodt]

I believe this should certainly have some perf impact now, altho it's deviated a bit from the original course of the PR

I think a lot of the original optimizations were on cold-paths, so they're not noticeable in the benchmarks but aren't bad either.
I attempted to fix something which is more on the hot-path, which was changing the functions on AstConv to no longer be dyn, which is likely slower and unnecessary as compared to implementing it on implementors of AstConv. I'm not sure about the style I have, but it should be faster.

[davidtwco]

Sorry for the delay in getting back to this, @JulianKnodt: I'll trigger another try and perf run, but could you rebase first (I think the try run will fail otherwise)?

[JulianKnodt]

No worries, it's the holidays and all, thank you for helping out even w/ me implementing somewhat random optimizations

[bors]

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

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking rustc_mir v0.0.0 (/checkout/compiler/rustc_mir)
    Checking rustc_passes v0.0.0 (/checkout/compiler/rustc_passes)
    Checking rustc_traits v0.0.0 (/checkout/compiler/rustc_traits)
    Checking rustc_mir_build v0.0.0 (/checkout/compiler/rustc_mir_build)
error[E0425]: cannot find function `check_generic_arg_count_for_call` in module `astconv::generics`
    --> compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs:1232:36
     |
1232 |               } = astconv::generics::check_generic_arg_count_for_call(
     |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `check_generic_arg_count`
    ::: compiler/rustc_typeck/src/astconv/generics.rs:338:1
     |
     |
338  | / pub(crate) fn check_generic_arg_count(
339  | |     tcx: TyCtxt<'_>,
340  | |     span: Span,
341  | |     def: &ty::Generics,
480  | |     }
481  | | }
481  | | }
     | |_- similarly named function `check_generic_arg_count` defined here

error[E0425]: cannot find function `check_generic_arg_count_for_call` in module `astconv::generics`
   --> compiler/rustc_typeck/src/check/method/confirm.rs:301:52
    |
301 |           let arg_count_correct = astconv::generics::check_generic_arg_count_for_call(
    |                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `check_generic_arg_count`
   ::: compiler/rustc_typeck/src/astconv/generics.rs:338:1
    |
    |
338 | / pub(crate) fn check_generic_arg_count(
339 | |     tcx: TyCtxt<'_>,
340 | |     span: Span,
341 | |     def: &ty::Generics,
480 | |     }
481 | | }
481 | | }
    | |_- similarly named function `check_generic_arg_count` defined here
    Checking rustc_plugin_impl v0.0.0 (/checkout/compiler/rustc_plugin_impl)
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0425`.
For more information about this error, try `rustc --explain E0425`.
error: could not compile `rustc_typeck`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" " llvm max_level_info" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "-p" "rustc-main" "-p" "rustc_driver" "-p" "rustc_lint" "-p" "rustc_index" "-p" "rustc_macros" "-p" "rustc_trait_selection" "-p" "rustc_infer" "-p" "rustc_graphviz" "-p" "rustc_parse_format" "-p" "rustc_lexer" "-p" "rustc_attr" "-p" "rustc_span" "-p" "rustc_arena" "-p" "rustc_save_analysis" "-p" "rustc_feature" "-p" "rustc_errors" "-p" "rustc_lint_defs" "-p" "rustc_ast" "-p" "rustc_middle" "-p" "rustc_query_system" "-p" "rustc_apfloat" "-p" "rustc_type_ir" "-p" "rustc_metadata" "-p" "rustc_expand" "-p" "rustc_ast_passes" "-p" "rustc_parse" "-p" "rustc_data_structures" "-p" "rustc_ast_pretty" "-p" "rustc_hir_pretty" "-p" "rustc_interface" "-p" "rustc_passes" "-p" "rustc_resolve" "-p" "rustc_codegen_llvm" "-p" "rustc_fs_util" "-p" "rustc_llvm" "-p" "rustc_typeck" "-p" "rustc_incremental" "-p" "rustc_traits" "-p" "rustc_privacy" "-p" "rustc_builtin_macros" "-p" "rustc_symbol_mangling" "-p" "rustc_mir_build" "-p" "rustc_ast_lowering" "-p" "rustc_ty_utils" "-p" "rustc_target" "-p" "rustc_mir" "-p" "coverage_test_macros" "-p" "rustc_session" "-p" "rustc_hir" "-p" "rustc_serialize" "-p" "rustc_error_codes" "-p" "rustc_plugin_impl" "-p" "rustc_codegen_ssa" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:02:51

[davidtwco]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

[bors]

⌛ Trying commit d6695bd with merge 61d0d1bc1c119080c4722bd97a803af5207e6ab5...

[bors]

☀️ Try build successful - checks-actions
Build commit: 61d0d1bc1c119080c4722bd97a803af5207e6ab5 (61d0d1bc1c119080c4722bd97a803af5207e6ab5)

[rust-timer]

Queued 61d0d1bc1c119080c4722bd97a803af5207e6ab5 with parent 90ccf4f, future comparison URL.

@rustbot label: +S-waiting-on-perf

[rust-timer]

Finished benchmarking try commit (61d0d1bc1c119080c4722bd97a803af5207e6ab5): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[JulianKnodt]

Let me know what you think of these results, some are p good but it's mixed.
If they're not significant I'll just toss out all the changes

[davidtwco]

Let me know what you think of these results, some are p good but it's mixed.
If they're not significant I'll just toss out all the changes

The max-rss results, while initially promising, are probably just noise (when compared to the last noise run). Unfortunately, I don't think these results are significant. Thanks for taking the time to try make an improvement here though!