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

Jump to bottom

stylo: various fixes to improve style logging in opt builds #15557

Merged
merged 2 commits into from Feb 15, 2017
Merged

stylo: various fixes to improve style logging in opt builds #15557

merged 2 commits into from Feb 15, 2017

Conversation

bholley
Copy link
Contributor

@bholley bholley commented Feb 15, 2017 •
edited by larsbergstrom

This adds a traversal time entry to the style statistics, and switches to warn! as discussed in [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1339176

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @emilio: components/style/properties/gecko.mako.rs, ports/geckolib/glue.rs, components/style/parallel.rs, components/style/restyle_hints.rs, components/style/sequential.rs, components/style/rule_tree/mod.rs, components/style/properties/helpers.mako.rs, components/style/values/specified/url.rs, components/style/context.rs

@highfive
Copy link

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 15, 2017
@bholley
Copy link
Contributor Author

bholley commented Feb 15, 2017

r? @emilio

@highfive highfive assigned emilio and unassigned glennw Feb 15, 2017
@bholley bholley mentioned this pull request Feb 15, 2017
Closed
emilio
emilio reviewed Feb 15, 2017
View reviewed changes
components/style/context.rs
}

/// Implementation of Add to aggregate statistics across different threads.
impl<'a> Add for &'a TraversalStatistics {
type Output = TraversalStatistics;
fn add(self, other: Self) -> TraversalStatistics {
debug_assert!(self.traversal_time_ms == 0.0 && other.traversal_time_ms == 0.0,
"traversal_time_ms should be set at the end by the caller");
Copy link
Member

@emilio emilio Feb 15, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me this shouldn't live in TraversalStatistics, though I see why you're doing it. I guess it's fine for now unless it becomes way more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just wanted it to show up in the same perf block for @shinglyu's automation, and this seemed like the simplest way to do it.

I agree it's a bit gross. The other way I considered was that each TraversalStatistics would capture a start time when instantiated, and then we'd take the min when we merge them, and rely on the fact that we must instantiate at least one ThreadLocalStyleContext (and therefore TraversalStatistics) very near to the beginning of the traversal. But that seemed like it required a lot of knowledge about the style system for people to verify that it was measuring the right thing, so I went with the dumb approach.

@emilio
Copy link
Member

emilio commented Feb 15, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit ae87b8a has been approved by emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 15, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit ae87b8a with merge 715a164...

bors-servo pushed a commit that referenced this pull request Feb 15, 2017
Auto merge of #15557 - bholley:opt_logging, r=emilio
715a164 
stylo: various fixes to improve style logging in opt builds

This adds a traversal time entry to the style statistics, and switches to warn! as discussed in [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1339176

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15557)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: emilio
Pushing 715a164 to master...

@bors-servo bors-servo merged commit ae87b8a into servo:master Feb 15, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio approved these changes

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bholley @highfive @emilio @bors-servo @glennw