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

Move script profiling to profile/profile_traits crates #7514

Closed
connorimes opened this issue Sep 2, 2015 · 3 comments
Closed

Move script profiling to profile/profile_traits crates #7514

connorimes opened this issue Sep 2, 2015 · 3 comments

Comments

@connorimes
Copy link
Contributor

@connorimes connorimes commented Sep 2, 2015

Profiling was recently added for script tasks. To better understand how servo is behaving on the whole, this should probably be centralized with other profiling capabilities in the profile/profile_traits crates.

The profile_event function itself can probably be dropped as well and just use the profile function in profile_traits. If you need to access total time for an event (as it does now), enabling heartbeats for the category will track this.

@benschulz

@jdm
Copy link
Member

@jdm jdm commented Sep 2, 2015

The original motivation of the script profiling was to get a sense of where script tasks in particular are spending their time during periods of slow script responsiveness. This also allows us quite a bit of granularity, so I have vague concerns about merging that in with the higher level profiling for all the rest of the code. Am I needlessly worried?

@connorimes
Copy link
Contributor Author

@connorimes connorimes commented Sep 2, 2015

Are you concerned about performance overhead or just separation of granularities? Some of the categories currently in the profiler seem pretty fine-grained, e.g. PaintingPerTile. Others seem to have very short runtimes, like LayoutGeneratedContent or Compositing, while others have long runtimes like LayoutStyleRecalc.

If we think a better structure and separation of categories is needed, that's OK, but I think they should still be centrally located. This supports better consistency in profiling behavior and less code maintenance. Also, having a single place to look for profiling data should make life easier for those of us interested in analyzing it.

@jdm
Copy link
Member

@jdm jdm commented Sep 2, 2015

It's the separation of granularities that matters to me.

bors-servo pushed a commit that referenced this issue Sep 8, 2015
Combine script profiling with profile crates. Fixes #7514.

The script crate had its own built-in profiling which was basically doing the same thing as the profile crate.  This wraps the internal profiling around the main profile functionality.  Script-related tasks are now added to the ProfilerCategory enum.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7547)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Sep 8, 2015
Combine script profiling with profile crates. Fixes #7514.

The script crate had its own built-in profiling which was basically doing the same thing as the profile crate.  This wraps the internal profiling around the main profile functionality.  Script-related tasks are now added to the ProfilerCategory enum.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7547)
<!-- Reviewable:end -->
@bors-servo bors-servo closed this in d746835 Sep 8, 2015
josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.