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

Avoid many `cmt` allocations. #50418

Merged
merged 1 commit into from May 5, 2018

Conversation

Projects
None yet
10 participants
@nnethercote
Copy link
Contributor

nnethercote commented May 3, 2018

cmt is a ref-counted wrapper around cmt_ The use of refcounting
keeps cmt handling simple, but a lot of cmt instances are very
short-lived, and heap-allocating the short-lived ones takes up time.

This patch changes things in the following ways.

  • Most of the functions that produced cmt instances now produce cmt_
    instances. The Rc::new calls that occurred within those functions
    now occur at their call sites (but only when necessary, which isn't
    that often).

  • Many of the functions that took cmt arguments now take &cmt_
    arguments. This includes all the methods in the Delegate trait.

As a result, the vast majority of the heap allocations are avoided. In
an extreme case, the number of calls to malloc in tuple-stress drops
from 9.9M to 7.9M, a drop of 20%. And the compile times for many runs of
coercions, deep-vector, and tuple-stress drop by 1--2%.

Avoid many `cmt` allocations.
`cmt` is a ref-counted wrapper around `cmt_` The use of refcounting
keeps `cmt` handling simple, but a lot of `cmt` instances are very
short-lived, and heap-allocating the short-lived ones takes up time.

This patch changes things in the following ways.

- Most of the functions that produced `cmt` instances now produce `cmt_`
  instances. The `Rc::new` calls that occurred within those functions
  now occur at their call sites (but only when necessary, which isn't
  that often).

- Many of the functions that took `cmt` arguments now take `&cmt_`
  arguments. This includes all the methods in the `Delegate` trait.

As a result, the vast majority of the heap allocations are avoided. In
an extreme case, the number of calls to malloc in tuple-stress drops
from 9.9M to 7.9M, a drop of 20%. And the compile times for many runs of
coercions, deep-vector, and tuple-stress drop by 1--2%.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 3, 2018

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@leonardo-m

This comment has been minimized.

Copy link

leonardo-m commented May 3, 2018

Could a compiler pass be added to perform this optimization (escape analysis?)?

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented May 3, 2018

LGTM. r? @petrochenkov or @eddyb for a second pair of eyes.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented May 3, 2018

Welp, all of this ancient code will hopefully go away once MIR borrowck takes over.
But for now, nice wins! @bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 3, 2018

📌 Commit 7cf142f has been approved by eddyb

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented May 3, 2018

@Manishearth: note that this will break Clippy. The fix will be easy, just updating method arguments for the changes in the Delegate trait.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented May 3, 2018

sounds good, thanks for the heads up!

@petrochenkov petrochenkov assigned eddyb and unassigned petrochenkov May 4, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 4, 2018

⌛️ Testing commit 7cf142f with merge f86297d...

bors added a commit that referenced this pull request May 4, 2018

Auto merge of #50418 - nnethercote:cmt, r=eddyb
Avoid many `cmt` allocations.

`cmt` is a ref-counted wrapper around `cmt_` The use of refcounting
keeps `cmt` handling simple, but a lot of `cmt` instances are very
short-lived, and heap-allocating the short-lived ones takes up time.

This patch changes things in the following ways.

- Most of the functions that produced `cmt` instances now produce `cmt_`
  instances. The `Rc::new` calls that occurred within those functions
  now occur at their call sites (but only when necessary, which isn't
  that often).

- Many of the functions that took `cmt` arguments now take `&cmt_`
  arguments. This includes all the methods in the `Delegate` trait.

As a result, the vast majority of the heap allocations are avoided. In
an extreme case, the number of calls to malloc in tuple-stress drops
from 9.9M to 7.9M, a drop of 20%. And the compile times for many runs of
coercions, deep-vector, and tuple-stress drop by 1--2%.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 4, 2018

💔 Test failed - status-appveyor

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented May 4, 2018

The failure is:

thread 'build_script::rename_with_link_search_path' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 5, kind: PermissionDenied, message: "Access is denied." }

Infrastructure failure? Worth a retry?

@phansch phansch referenced this pull request May 4, 2018

Closed

Incoming clippy breakage #2725

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented May 4, 2018

@bors retry

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented May 4, 2018

cc #48775

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 5, 2018

⌛️ Testing commit 7cf142f with merge a19e9ec...

bors added a commit that referenced this pull request May 5, 2018

Auto merge of #50418 - nnethercote:cmt, r=eddyb
Avoid many `cmt` allocations.

`cmt` is a ref-counted wrapper around `cmt_` The use of refcounting
keeps `cmt` handling simple, but a lot of `cmt` instances are very
short-lived, and heap-allocating the short-lived ones takes up time.

This patch changes things in the following ways.

- Most of the functions that produced `cmt` instances now produce `cmt_`
  instances. The `Rc::new` calls that occurred within those functions
  now occur at their call sites (but only when necessary, which isn't
  that often).

- Many of the functions that took `cmt` arguments now take `&cmt_`
  arguments. This includes all the methods in the `Delegate` trait.

As a result, the vast majority of the heap allocations are avoided. In
an extreme case, the number of calls to malloc in tuple-stress drops
from 9.9M to 7.9M, a drop of 20%. And the compile times for many runs of
coercions, deep-vector, and tuple-stress drop by 1--2%.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 5, 2018

💔 Test failed - status-travis

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented May 5, 2018

The job arm-android of your PR failed on Travis (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.
[01:35:27] test time::tests::system_time_elapsed ... ok
[01:35:27] test time::tests::system_time_math ... ok
[01:35:32] test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok
[01:35:35] test collections::hash::map::test_map::test_lots_of_insertions ... ok
[01:36:07] test process::tests::test_process_output_fail_to_start ... test process::tests::test_process_output_fail_to_start has been running for over 60 seconds
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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)

1 similar comment
@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented May 5, 2018

The job arm-android of your PR failed on Travis (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.
[01:35:27] test time::tests::system_time_elapsed ... ok
[01:35:27] test time::tests::system_time_math ... ok
[01:35:32] test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok
[01:35:35] test collections::hash::map::test_map::test_lots_of_insertions ... ok
[01:36:07] test process::tests::test_process_output_fail_to_start ... test process::tests::test_process_output_fail_to_start has been running for over 60 seconds
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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)

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented May 5, 2018

[01:36:07] test process::tests::test_process_output_fail_to_start ... test process::tests::test_process_output_fail_to_start has been running for over 60 seconds

Which is #43283.

Can somebody please retry?

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented May 5, 2018

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 5, 2018

💡 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: #50423
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 5, 2018

📌 Commit 7cf142f has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 5, 2018

⌛️ Testing commit 7cf142f with merge e471c20...

bors added a commit that referenced this pull request May 5, 2018

Auto merge of #50418 - nnethercote:cmt, r=eddyb
Avoid many `cmt` allocations.

`cmt` is a ref-counted wrapper around `cmt_` The use of refcounting
keeps `cmt` handling simple, but a lot of `cmt` instances are very
short-lived, and heap-allocating the short-lived ones takes up time.

This patch changes things in the following ways.

- Most of the functions that produced `cmt` instances now produce `cmt_`
  instances. The `Rc::new` calls that occurred within those functions
  now occur at their call sites (but only when necessary, which isn't
  that often).

- Many of the functions that took `cmt` arguments now take `&cmt_`
  arguments. This includes all the methods in the `Delegate` trait.

As a result, the vast majority of the heap allocations are avoided. In
an extreme case, the number of calls to malloc in tuple-stress drops
from 9.9M to 7.9M, a drop of 20%. And the compile times for many runs of
coercions, deep-vector, and tuple-stress drop by 1--2%.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing e471c20 to master...

@bors bors merged commit 7cf142f into rust-lang:master May 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:cmt branch May 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.