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

Use a `SmallVec` for `Candidate::match_pairs`. #66540

Merged
merged 1 commit into from Nov 25, 2019

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Nov 19, 2019

This is a small win for encoding.

r? @matthewjasper

This is a small win for `encoding`.
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 19, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 19, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

⌛️ Trying commit 4c33a5a with merge 30e1b2a...

bors added a commit that referenced this pull request Nov 19, 2019
…<try>

Use a `SmallVec` for `Candidate::match_pairs`.

This is a small win for `encoding`.

r? @matthewjasper
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

☀️ Try build successful - checks-azure
Build commit: 30e1b2a (30e1b2a64ad4db32db1ef28cee4f5fad8efc61dd)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 19, 2019

Queued 30e1b2a with parent 2cad8bb, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 19, 2019

Finished benchmarking try commit 30e1b2a, comparison URL.

@bjorn3

This comment has been minimized.

Copy link
Contributor

bjorn3 commented Nov 19, 2019

Only noise.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Nov 19, 2019

There is a small signal in the noise, I promise! Let me explain. For an encoding-check-clean build Callgrind tells me that 14.85% of instructions executed are within malloc.c, i.e. allocations and deallocations. And DHAT tells me that the single biggest source of allocations is here:

 Total:     798,080 bytes (0.54%, 544.46/Minstr) in 24,728 blocks (2.68%, 16.87/Minstr), avg size 32.27 bytes, avg lifetime 60,211.12 instrs (0% of program duration)
 Max:       7,008 bytes in 219 blocks, avg size 32 bytes
 At t-gmax: 0 bytes (0%) in 0 blocks (0%), avg size 0 bytes
 At t-end:  0 bytes (0%) in 0 blocks (0%), avg size 0 bytes
 Reads:     852,604 bytes (0.16%, 581.66/Minstr), 1.07/byte
 Writes:    801,120 bytes (0.42%, 546.54/Minstr), 1/byte
 Allocated at {
   ^1: 0x51EEA4B: alloc (alloc.rs:84)
   ^2: 0x51EEA4B: alloc (alloc.rs:172)
   ^3: 0x51EEA4B: reserve_internal<(syntax_pos::span_encoding::Span, alloc::string::String),alloc::alloc::Global> (raw_vec.rs:695)
   ^4: 0x51EEA4B: reserve<(syntax_pos::span_encoding::Span, alloc::string::String),alloc::alloc::Global> (raw_vec.rs:520)
   ^5: 0x51EEA4B: alloc::vec::Vec<T>::reserve (vec.rs:501)
   #6: 0x545C9E3: push<rustc_mir::build::matches::MatchPair> (vec.rs:1145)
   #7: 0x545C9E3: rustc_mir::build::matches::simplify::<impl rustc_mir::build::Builder>::simplify_candidate (simplify.rs:39)
   #8: 0x5464F6B: rustc_mir::build::matches::<impl rustc_mir::build::Builder>::match_candidates (mod.rs:826)
 }

Changing to a SmallVec eliminates over 99% of these allocations, reducing the total number of allocations by 2.67%. This translates to a 0.2% instruction count reduction. Which is small, but the patch is so simple that I figure it's worth landing.

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 24, 2019

Ping from triage.
@matthewjasper - this PR is still waiting on review.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Nov 24, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

📌 Commit 4c33a5a has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

⌛️ Testing commit 4c33a5a with merge 082c648...

bors added a commit that referenced this pull request Nov 24, 2019
…matthewjasper

Use a `SmallVec` for `Candidate::match_pairs`.

This is a small win for `encoding`.

r? @matthewjasper
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 24, 2019

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-24T17:27:54.1738085Z do so (now or later) by using -b with the checkout command again. Example:
2019-11-24T17:27:54.1739356Z 
2019-11-24T17:27:54.1739588Z   git checkout -b <new-branch-name>
2019-11-24T17:27:54.1739800Z 
2019-11-24T17:27:54.1740026Z HEAD is now at 082c6481e Auto merge of #66540 - nnethercote:SmallVec-Candidate-match_pairs, r=matthewjasper
2019-11-24T17:27:54.2144215Z ##[section]Starting: Decide whether to run this job
2019-11-24T17:27:54.2242619Z ==============================================================================
2019-11-24T17:27:54.2242708Z Task         : Bash
2019-11-24T17:27:54.2242783Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-11-24T17:27:55.4969342Z BUILD_SOURCEBRANCHNAME=auto
2019-11-24T17:27:55.4969436Z BUILD_SOURCESDIRECTORY=D:\a\1\s
2019-11-24T17:27:55.4969532Z BUILD_SOURCEVERSION=082c6481eaf5953d4e8ab51a223747859c64d51b
2019-11-24T17:27:55.4969638Z BUILD_SOURCEVERSIONAUTHOR=bors
2019-11-24T17:27:55.4969779Z BUILD_SOURCEVERSIONMESSAGE=Auto merge of #66540 - nnethercote:SmallVec-Candidate-match_pairs, r=matthewjasper
2019-11-24T17:27:55.4970065Z CI_JOB_NAME=x86_64-msvc-1
2019-11-24T17:27:55.4970146Z COBERTURA_HOME=C:\cobertura-2.1.1
2019-11-24T17:27:55.4970251Z COMMONPROGRAMFILES=C:\Program Files\Common Files
2019-11-24T17:27:55.4970343Z COMMON_TESTRESULTSDIRECTORY=D:\a\1\TestResults
---
2019-11-24T17:27:55.4985403Z USERDOMAIN=fv-az172
2019-11-24T17:27:55.4985492Z USERDOMAIN_ROAMINGPROFILE=fv-az172
2019-11-24T17:27:55.4985580Z USERNAME=VssAdministrator
2019-11-24T17:27:55.4985674Z USERPROFILE=C:\Users\VssAdministrator
2019-11-24T17:27:55.4985762Z Use a `SmallVec` for `Candidate::match_pairs`.
2019-11-24T17:27:55.4986026Z VS140COMNTOOLS=C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\
2019-11-24T17:27:55.4986131Z VSTS_AGENT_PERFLOG=c:\vsts\perflog
2019-11-24T17:27:55.4986241Z VSTS_PROCESS_LOOKUP_ID=vsts_7422ad9a-1773-4e81-b4e8-c2485db483e2
2019-11-24T17:27:55.4986329Z WINDIR=C:\windows
---
2019-11-24T17:30:27.6250150Z 
2019-11-24T17:30:27.6250292Z 
2019-11-24T17:30:27.6880184Z kill: 1600: No such process
2019-11-24T17:30:27.6884039Z kill: 1599: No such process
2019-11-24T18:14:34.2477517Z Chocolatey timed out waiting for the command to finish. The timeout 
2019-11-24T18:14:34.2478154Z  specified (or the default value) was '2700' seconds. Perhaps try a 
2019-11-24T18:14:34.2478317Z  higher `--execution-timeout`? See `choco -h` for details.
2019-11-24T18:14:35.0759296Z The install of msys2 was NOT successful.
2019-11-24T18:14:35.0760784Z Error while running 'C:\ProgramData\chocolatey\lib\msys2\tools\chocolateyinstall.ps1'.
2019-11-24T18:14:35.0761420Z  See log for details.
2019-11-24T18:14:37.2064139Z Chocolatey installed 0/1 packages. 1 packages failed.
2019-11-24T18:14:37.2064400Z  See the log for details (C:\ProgramData\chocolatey\logs\chocolatey.log).
2019-11-24T18:14:37.2068465Z 
2019-11-24T18:14:37.2074365Z Failures
2019-11-24T18:14:37.2074365Z Failures
2019-11-24T18:14:37.2081174Z  - msys2 (exited -1) - Error while running 'C:\ProgramData\chocolatey\lib\msys2\tools\chocolateyinstall.ps1'.
2019-11-24T18:14:37.2081536Z  See log for details.
2019-11-24T18:14:47.6596291Z The STDIO streams did not close within 10 seconds of the exit event from process 'C:\Program Files\Git\bin\bash.exe'. This may indicate a child process inherited the STDIO streams and has not yet exited.
2019-11-24T18:14:47.6686013Z ##[error]Bash exited with code '127'.
2019-11-24T18:14:52.6889956Z ##[section]Starting: Checkout
2019-11-24T18:14:52.7019819Z ==============================================================================
2019-11-24T18:14:52.7019976Z Task         : Get sources
2019-11-24T18:14:52.7020074Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

💔 Test failed - checks-azure

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Nov 24, 2019

Unclear what went wrong... nothing obvious to me in the log, and this patch is extremely simple. Maybe just a CI glitch? Let's try again.

@bors r=nnethercote

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: #66647
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

📌 Commit 4c33a5a has been approved by nnethercote

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Nov 24, 2019

@bors r=matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #66647
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

📌 Commit 4c33a5a has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

⌛️ Testing commit 4c33a5a with merge 388ffd9...

bors added a commit that referenced this pull request Nov 24, 2019
…matthewjasper

Use a `SmallVec` for `Candidate::match_pairs`.

This is a small win for `encoding`.

r? @matthewjasper
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 25, 2019

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 388ffd9 to master...

@bors bors added the merged-by-bors label Nov 25, 2019
@bors bors merged commit 4c33a5a into rust-lang:master Nov 25, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr #20191119.8 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:SmallVec-Candidate-match_pairs branch Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.