Skip to content
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

Remove the `DelimSpan` from `NamedMatch::MatchedSeq`. #67250

Merged

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Dec 12, 2019

Because it's unused. This then allows the removal of
MatcherPos::sp_open. It's a tiny perf win, reducing instruction counts
by 0.1% - 0.2% on a few benchmarks.

r? @Centril

Because it's unused. This then allows the removal of
`MatcherPos::sp_open`. It's a tiny perf win, reducing instruction counts
by 0.1% - 0.2% on a few benchmarks.
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 12, 2019

I found this by profiling serde_derive (which isn't in rustc-perf) with DHAT hacked to detect memcpy calls. The commit shrinks NamedMatchVec from 136 bytes to something much smaller (72 bytes at a guess, though I haven't checked). That's less than the 128 byte threshold above which memcpy is used for copies.

For a check-clean build of serde_derive the commit reduces the number of memcpy calls from 12M to 9.6M, and the number of memcpyd bytes from 1.91B to 1.59B.

Here is one of the DHAT records. There were two other very similar ones.

  │     Total:     107,143,656 bytes (5.6%, 9,030.59/Minstr) in 787,821 blocks (6.57%, 66.4/Minstr), avg size 136 bytes, avg lifetime 0 instrs (0% of program duration)
  │     At t-gmax: 107,143,656 bytes (5.6%) in 787,821 blocks (6.57%), avg size 136 bytes
  │     At t-end:  107,143,656 bytes (5.6%) in 787,821 blocks (6.57%), avg size 136 bytes
  │     Reads:     0 bytes (0%, 0/Minstr), 0/byte
  │     Writes:    0 bytes (0%, 0/Minstr), 0/byte
  │     Allocated at {
  │       ^1: 0x483B247: memmove (dh_replace_memcpy.c:227)
  │       ^2: 0x5A5CE52: clone<[syntax_expand::mbe::macro_parser::NamedMatch; 4]> (lib.rs:1454)
  │       ^3: 0x5A5CE52: alloc::rc::Rc<T>::make_mut (rc.rs:857)
  │       #4: 0x5ADB4F6: syntax_expand::mbe::macro_parser::MatcherPos::push_match (macro_parser.rs:223)
  │     }
@nnethercote nnethercote mentioned this pull request Dec 12, 2019
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 12, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 12, 2019

📌 Commit ffd2142 has been approved by Centril

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 12, 2019

Centril added a commit to Centril/rust that referenced this pull request Dec 12, 2019
…Match-MatchedSeq, r=Centril

Remove the `DelimSpan` from `NamedMatch::MatchedSeq`.

Because it's unused. This then allows the removal of
`MatcherPos::sp_open`. It's a tiny perf win, reducing instruction counts
by 0.1% - 0.2% on a few benchmarks.

r? @Centril
bors added a commit that referenced this pull request Dec 13, 2019
Rollup of 7 pull requests

Successful merges:

 - #67224 (Revert stabilization of never type)
 - #67235 (VecDeque: drop remaining items on destructor panic)
 - #67243 (LinkedList: drop remaining items when drop panics)
 - #67247 (Don't suggest wrong snippet in closure)
 - #67250 (Remove the `DelimSpan` from `NamedMatch::MatchedSeq`.)
 - #67251 (Require `allow_internal_unstable` for stable min_const_fn using unsta…)
 - #67255 (Remove i686-unknown-dragonfly target)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Dec 13, 2019
…Match-MatchedSeq, r=Centril

Remove the `DelimSpan` from `NamedMatch::MatchedSeq`.

Because it's unused. This then allows the removal of
`MatcherPos::sp_open`. It's a tiny perf win, reducing instruction counts
by 0.1% - 0.2% on a few benchmarks.

r? @Centril
bors added a commit that referenced this pull request Dec 13, 2019
Rollup of 6 pull requests

Successful merges:

 - #66341 (Match `VecDeque::extend` to `Vec::extend_desugared`)
 - #67243 (LinkedList: drop remaining items when drop panics)
 - #67247 (Don't suggest wrong snippet in closure)
 - #67250 (Remove the `DelimSpan` from `NamedMatch::MatchedSeq`.)
 - #67251 (Require `allow_internal_unstable` for stable min_const_fn using unsta…)
 - #67269 (parser: recover on `&'lifetime mut? $pat`.)

Failed merges:

r? @ghost
@bors bors merged commit ffd2142 into rust-lang:master Dec 13, 2019
4 checks passed
4 checks passed
pr #20191212.12 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:rm-DelimSpan-from-NamedMatch-MatchedSeq branch Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.