Skip to content

Put mtime-on-use behind a feature flag. #6573

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jan 20, 2019

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 20, 2019

This places #6477 behind the -Z mtime-on-use feature flag.

The change to update the mtime each time a crate is used has caused a performance regression on the rust playground (rust-lang/rust#57774). It is using about 241 pre-built crates in a Docker container. Due to the copy-on-write nature of Docker, it can take a significant amount of time to update the timestamps (over 10 seconds on slower systems).

cc @Mark-Simulacrum

@rust-highfive
Copy link

r? @dwijnand

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

@dwijnand
Copy link
Member

SGTM. I think fixing a performance regression trumps here.
@bors r+

@bors
Copy link
Contributor

bors commented Jan 20, 2019

📌 Commit 5f6ede2 has been approved by dwijnand

@bors
Copy link
Contributor

bors commented Jan 20, 2019

⌛ Testing commit 5f6ede2 with merge 907c0fe...

bors added a commit that referenced this pull request Jan 20, 2019
Put mtime-on-use behind a feature flag.

This places #6477 behind the `-Z mtime-on-use` feature flag.

The change to update the mtime each time a crate is used has caused a performance regression on the rust playground (rust-lang/rust#57774). It is using about 241 pre-built crates in a Docker container. Due to the copy-on-write nature of Docker, it can take a significant amount of time to update the timestamps (over 10 seconds on slower systems).

cc @Mark-Simulacrum
@bors
Copy link
Contributor

bors commented Jan 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: dwijnand
Pushing 907c0fe to master...

@bors bors merged commit 5f6ede2 into rust-lang:master Jan 20, 2019
bors added a commit to rust-lang/rust that referenced this pull request Jan 21, 2019
Update cargo

Pull in fix for #57774.

6 commits in ffe65875fd05018599ad07e7389e99050c7915be..907c0febe7045fa02dff2a35c5e36d3bd59ea50d
2019-01-17 23:57:50 +0000 to 2019-01-20 22:31:07 +0000
- Put mtime-on-use behind a feature flag. (rust-lang/cargo#6573)
- Fix a typo in the unstable docs (rust-lang/cargo#6569)
- Perhaps you meant: foo, bar or foobar (rust-lang/cargo#6550)
- Refactor: Create uninstall submodule (rust-lang/cargo#6557)
- Fix spurious Windows errors with switch_features_rerun. (rust-lang/cargo#6561)
- Stop building on master on Travis. (rust-lang/cargo#6562)

r? @Mark-Simulacrum
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 24, 2019

Thank you for the fast fix.

In the long run I think this will be easier to experiment with as an environment variable (that can be set once) then as a flag (that needs to be set on each invocation). cc rust-lang/crates.io#1602.

Have we used unstable environment variables in cargo in the past? Similar to how the rollout of incremental compilation had a environment variable. How should they be set up? I am happy to do the work, just could use some guidance.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants