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

Integrate with simple Heartbeats #7179

Merged
merged 1 commit into from Aug 22, 2015
Merged

Conversation

@connorimes
Copy link
Contributor

connorimes commented Aug 12, 2015

This PR adds Heartbeats capability to servo. Heartbeats are used for detailed performance and power/energy profiling. We will add the power/energy readings in the future.

New dependencies are introduced which need in-depth reviews. I'm the only one who has had eyes on any of this, and I have limited resources for testing cross-platform compatibility.

The new heartbeats module in the profile crate looks for environment variables telling it to use heartbeats for each ProfilerCategory and where to put log files. (Of course, if somebody knows how to iterate over the enum instead of hardcoding each one, that would be fantastic.) If the environment variables aren't set for particular categories, heartbeats aren't created or used.

An interface change is made in the profile_traits crate to pass both the start and end time in a ProfilerMsg instead of just the elapsed time. Later we will add energy readings as well.

Review on Reviewable

@highfive
Copy link

highfive commented Aug 12, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@connorimes
Copy link
Contributor Author

connorimes commented Aug 12, 2015

I expect this review to be an iterative process, so I will continue with bug fixes and improvements until you ask for a code freeze, at which point I'll squash.

@larsbergstrom larsbergstrom self-assigned this Aug 14, 2015
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 17, 2015

Does this work on other platforms? I'm especially concerned about 32-bit Android. There are instructions to build at: https://github.com/servo/servo/wiki/Building-for-Android


Reviewed 7 of 8 files at r1, 1 of 1 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/profile/heartbeats.rs, line 16 [r1] (raw file):
Can you use the lazy_static support instead? http://kimundi.github.io/lazy-static.rs/lazy_static/index.html I believe it would remove the unsafe code in init by allowing you to move the HashMap creation to the lazy static and just add the items in init


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2015

The latest upstream changes (presumably #7243) made this pull request unmergeable. Please resolve the merge conflicts.

@connorimes connorimes force-pushed the connorimes:add-heartbeats branch from d764dba to 595a9d7 Aug 20, 2015
@connorimes
Copy link
Contributor Author

connorimes commented Aug 20, 2015

Review status: 3 of 8 files reviewed at latest revision, all discussions resolved, all commit checks successful.


components/profile/heartbeats.rs, line 16 [r1] (raw file):
lazy_static requires Sync trait, but the HashMap values are of Heartbeat type which wraps some FFI types which are not Sync.
Do you have a good way around this?
FYI, I think we still need a cleanup function to force the Heartbeats to drop and flush any remaining log data to files. Otherwise, I believe static data does not drop when the application finishes, but please correct me if I'm wrong.


Comments from the review on Reviewable.io

@connorimes
Copy link
Contributor Author

connorimes commented Aug 20, 2015

I've tested this locally with an android toolchain and it compiles. I haven't tried to run it in an emulator though.
There aren't any runtime dependencies for the heartbeats native libraries. The only dependency that appears in heartbeats-simple is librt in example code which obviously we don't use. Hopefully this means the libraries should should just work assuming the build environment is setup properly.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 21, 2015

Reviewed 5 of 5 files at r4.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 21, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2015

📌 Commit 595a9d7 has been approved by larsbergstrom

@connorimes connorimes force-pushed the connorimes:add-heartbeats branch from 4a6cdf0 to 054cbf2 Aug 21, 2015
@jdm
Copy link
Member

jdm commented Aug 22, 2015

@bors-servo: r=larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2015

📌 Commit 054cbf2 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2015

Testing commit 054cbf2 with merge d89e4f7...

bors-servo pushed a commit that referenced this pull request Aug 22, 2015
Integrate with simple Heartbeats

This PR adds Heartbeats capability to servo.  Heartbeats are used for detailed performance and power/energy profiling.  We will add the power/energy readings in the future.

New dependencies are introduced which need in-depth reviews.  I'm the only one who has had eyes on any of this, and I have limited resources for testing cross-platform compatibility.
* https://github.com/libheartbeats/heartbeats-simple - provides native C libraries from a shared code base:
 * hbs[-static] - performance monitoring
 * hbs-acc[-static] - performance with accuracy monitoring
 * hbs-pow[-static] - performance with power/energy monitoring (the one we're using)
 * hbs-acc-pow[-static] - performance with accuracy and power/energy monitoring 
* https://github.com/connorimes/heartbeats-simple-sys provides rust wrappers for the native C libraries above - one crate for each + a common crate.  These link with the *-static versions of the heartbeats libraries.
* https://github.com/connorimes/heartbeats-simple-rust provides rust abstractions over the -sys crates above - one crate for each.

The new `heartbeats` module in the `profile` crate looks for environment variables telling it to use heartbeats for each ProfilerCategory and where to put log files.  (Of course, if somebody knows how to iterate over the enum instead of hardcoding each one, that would be fantastic.)  If the environment variables aren't set for particular categories, heartbeats aren't created or used.

An interface change is made in the `profile_traits` crate to pass both the start and end time in a `ProfilerMsg` instead of just the elapsed time.  Later we will add energy readings as well.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7179)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3

@bors-servo bors-servo merged commit 054cbf2 into servo:master Aug 22, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@SimonSapin SimonSapin mentioned this pull request May 26, 2020
5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.