Skip to content

Adjust style-servo patch #728

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

Merged

Conversation

Mark-Simulacrum
Copy link
Member

This adjusts the style-servo println patch to be in a non-templated file.
Previously, the properties file change would cause the build script to rerun.

This adjusts the style-servo println patch to be in a non-templated file.
Previously, the properties file change would cause the build script to rerun.
@Mark-Simulacrum
Copy link
Member Author

We'll likely want to re-collect stable (i.e., dashboard) benchmarks to account for this adjustment to style-servo, but that can be done later when our queue isn't suffering quite as much :)

@Mark-Simulacrum
Copy link
Member Author

I suspect that the old patch was technically still fine -- it caused a single-line change to the output of the template. But editing a 141,865 line file means that our current codegen unit partioning strategy is going to recompile a lot more as a result.

@nnethercote I'm going to let you weigh in before we merge this (or not). IMO, this PR brings the patch more inline with what we expect from the println patches -- small edits to small CGUs. Alternatively we can call this a bug in rustc that a small edit to a large module represents a lot of recompilation (maybe related to the generated nature of the file, i.e., that it's all included via a include! macro?).

cc @oli-obk @wesleywiser @pnkfelix who I believe are currently working on incremental / CGU partioning, but feel free to cc others if I've categorized incorrectly.

@Mark-Simulacrum
Copy link
Member Author

For reference, here is the generated file: https://gist.github.com/Mark-Simulacrum/9fe9b6fde89c4598c4ae0c13f45bad12

@nnethercote
Copy link
Contributor

I don't have a strong preference here. Changing the println location seems reasonable.

@Mark-Simulacrum
Copy link
Member Author

Okay, I'll go ahead and merge this then. It should be a slight increase in the instructions stat for the style-servo incremental tests but not as significant as what we saw the drop as.

I'll also work on recollecting stable builds.

@Mark-Simulacrum Mark-Simulacrum merged commit f53d94e into rust-lang:master Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants