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

Show IPC profile markers #2172

Merged
merged 3 commits into from Oct 22, 2019

Conversation

@jimporter
Copy link
Contributor

jimporter commented Jul 26, 2019

This resolves #2164.

@julienw

This comment has been minimized.

Copy link
Contributor

julienw commented Jul 30, 2019

hey @jimporter, do you need some feedback on this already, or do you plan some more work before this?
Thanks!

@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Jul 30, 2019

@julienw I do plan to keep working on this, but some feedback on the profile processing bits would be much appreciated! I'll be out Wed-Fri, but I'll get back to working on this in earnest on Monday.

@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Aug 13, 2019

@gregtatum Ok, I updated this patch to do the IPC marker correlation via a selector. It's a big mess right now, but I wanted to make sure I was headed in the right direction with this before I spent too long cleaning things up.

Note: currently, the change to using a selector is just a commit on top of the previous one; that's just so I don't lose the prior commit while I'm figuring out how to do this the right way. If the current code looks basically good, I'll squash the commits and get rid of the bad old version.

Copy link
Member

gregtatum left a comment

This looks like the correct approach! I went ahead and did a more thorough review while I was looking at the code, although I know it was a quick implementation. I hope my feedback is not annoying! I'm only marking this review as a "Comment", since everything is still a WIP.

The blockers for landing I see are:

  • Reducing algorithmic complexity
  • Adding types definitions for everything
  • Tests.
src/profile-logic/marker-data.js Outdated Show resolved Hide resolved
src/profile-logic/marker-data.js Outdated Show resolved Hide resolved
src/selectors/per-thread/markers.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
@jimporter jimporter force-pushed the jimporter:ipc-markers branch from 07f8e00 to cb4d3f2 Aug 13, 2019
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Aug 13, 2019

@gregtatum I reworked the selector to build a lookup table for the recipient IPC markers instead of the PID->thread mapping. This takes more memory, but it should change the selector from (roughly) O(n^2) to O(n). There's still a bunch more to do on improving the selector (e.g. making the Map key generation more-performant), but I'm going to do that after some fixes to the Gecko patch.

@jimporter jimporter force-pushed the jimporter:ipc-markers branch 2 times, most recently from f8f5a0f to cb4acf5 Aug 15, 2019
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Aug 21, 2019

Here's a profile I generated that you can use to try this PR out: https://deploy-preview-2172--perf-html.netlify.com/public/6c8aceb16b8dbe0c525fa2f12307aaae19d05d09

(e: fixed link to point to the preview build)

@jimporter jimporter force-pushed the jimporter:ipc-markers branch from cb4acf5 to 0bbf4e8 Aug 21, 2019
@jimporter jimporter force-pushed the jimporter:ipc-markers branch from 0bbf4e8 to 40c1044 Sep 3, 2019
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Sep 3, 2019

@gregtatum, I think all that's left here is to add some tests, and possibly tweak the UI based on feedback from people trying this out. Aside from that, does this look about right?

Copy link
Member

gregtatum left a comment

Thanks, this is all looking like it's right on track! I left a bunch of smaller comments on some suggested changes to the code. None of it was really big architectural things.

I would say we should currently block landing for the following:

  • Tests
  • Code documentation, and some slight file refactoring (I've outlined it in my comments).

General UI Feedback

  • It would probably be good to surface some text in the marker chart around the IPC marker. I believe UserTimings do something similar already.
  • Right now this is a really disruptive change to the marker chart as it adds lots of visual space. If this is not on by default, then this is probably fine. However, it is on by default we should probably discuss some way to mitigate it.
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/selectors/profile.js Outdated Show resolved Hide resolved
src/profile-logic/marker-data.js Outdated Show resolved Hide resolved
src/profile-logic/marker-styles.js Show resolved Hide resolved
src/profile-logic/marker-styles.js Outdated Show resolved Hide resolved
@jimporter jimporter force-pushed the jimporter:ipc-markers branch from 40c1044 to 11bb56f Sep 9, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 9, 2019

Codecov Report

Merging #2172 into master will decrease coverage by 0.36%.
The diff coverage is 30.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2172      +/-   ##
==========================================
- Coverage   85.98%   85.61%   -0.37%     
==========================================
  Files         200      200              
  Lines       14197    14061     -136     
  Branches     3571     3550      -21     
==========================================
- Hits        12207    12039     -168     
- Misses       1823     1849      +26     
- Partials      167      173       +6
Impacted Files Coverage Δ
src/profile-logic/marker-styles.js 100% <ø> (ø) ⬆️
src/components/tooltip/Marker.js 84.11% <0%> (-1.81%) ⬇️
src/selectors/per-thread/markers.js 97.43% <100%> (+0.03%) ⬆️
src/selectors/profile.js 85.31% <44.73%> (-11.22%) ⬇️
src/profile-logic/marker-data.js 85.03% <5.26%> (-3.91%) ⬇️
src/profile-logic/js-tracer.js 91.8% <0%> (-0.77%) ⬇️
src/selectors/per-thread/thread.js 98% <0%> (-0.34%) ⬇️
src/profile-logic/sanitize.js 98.76% <0%> (-0.18%) ⬇️
src/profile-logic/process-profile.js 93.7% <0%> (-0.1%) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 107c044...11bb56f. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 9, 2019

Codecov Report

Merging #2172 into master will increase coverage by 0.03%.
The diff coverage is 89.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2172      +/-   ##
==========================================
+ Coverage   86.11%   86.14%   +0.03%     
==========================================
  Files         203      204       +1     
  Lines       14828    14958     +130     
  Branches     3717     3749      +32     
==========================================
+ Hits        12769    12886     +117     
- Misses       1887     1900      +13     
  Partials      172      172
Impacted Files Coverage Δ
src/profile-logic/marker-styles.js 100% <ø> (ø) ⬆️
src/selectors/app.js 59.15% <0%> (-2.61%) ⬇️
src/actions/profile-view.js 89.67% <0%> (-1.08%) ⬇️
src/app-logic/constants.js 100% <100%> (ø) ⬆️
src/selectors/per-thread/markers.js 97.4% <100%> (+0.14%) ⬆️
src/selectors/profile.js 96.75% <100%> (+0.02%) ⬆️
src/test/fixtures/profiles/gecko-profile.js 100% <100%> (ø) ⬆️
src/profile-logic/tracks.js 91.86% <100%> (+0.11%) ⬆️
src/components/timeline/Markers.js 83.73% <100%> (+0.6%) ⬆️
src/components/timeline/LocalTrack.js 90.76% <100%> (+1.48%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5714dc2...58736a9. Read the comment docs.

@jimporter jimporter force-pushed the jimporter:ipc-markers branch 3 times, most recently from cc32002 to 77f5544 Sep 9, 2019
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Sep 9, 2019

@gregtatum Ok, I think I've resolved everything except the visual issues and the tests. Thanks for taking a look at this!

If you have any thoughts about how I can improve the UI further (e.g. putting these markers somewhere special instead of in the generic marker timeline), just let me know. I'll keep experimenting with some alternate UIs on my end as well.

@jimporter jimporter force-pushed the jimporter:ipc-markers branch 7 times, most recently from 833f51d to f8aff1b Sep 9, 2019
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Sep 12, 2019

@gregtatum I think I've addressed all the issues here. I do plan to work on a better UI for the timeline later, though I didn't want this to get too bogged down in UI tinkering before we have anyone else trying to use this. Adding a special timeline track for IPC messages would probably be good, though I think the specifics of what it would look like depend on people's needs...

@jimporter jimporter force-pushed the jimporter:ipc-markers branch from f8aff1b to 1b7f2d6 Sep 19, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 18, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

--HG--
extra : moz-landing-system : lando
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Oct 18, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 19, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: cd4dc0ae3364a2216df8ba7598d8cddcaf4944b1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 19, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: 2923afce519ae4339e8fb7d29dfba065717eff83
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 19, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: cd4dc0ae3364a2216df8ba7598d8cddcaf4944b1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 19, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: 2923afce519ae4339e8fb7d29dfba065717eff83
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 19, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: cd4dc0ae3364a2216df8ba7598d8cddcaf4944b1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 19, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: 2923afce519ae4339e8fb7d29dfba065717eff83
Copy link
Member

gregtatum left a comment

I'm marking "request changes" so that it's obvious what the status is here. My feedback is mostly open ended. I think it might be helpful to chat real-time about this in slack.

src/components/timeline/Markers.js Show resolved Hide resolved
src/components/timeline/TrackIPC.css Outdated Show resolved Hide resolved
src/components/timeline/TrackIPC.js Show resolved Hide resolved
src/components/tooltip/Marker.js Show resolved Hide resolved
src/profile-logic/tracks.js Outdated Show resolved Hide resolved
@jimporter jimporter force-pushed the jimporter:ipc-markers branch from 8716423 to c767c2e Oct 21, 2019
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Oct 21, 2019

Here's a new profile to test the UI with (this adds the sync attribute to IPC markers): https://deploy-preview-2172--perf-html.netlify.com/public/dab7795ca9494375f62e4adfe7826e94633825aa

@jimporter jimporter force-pushed the jimporter:ipc-markers branch from c767c2e to c876edb Oct 21, 2019
* Src/dst IPC markers are correlated with each other via a selector, letting
  us show the queuing time for an IPC message
* IPC marker data is shown on the marker tracks for each thread as well as in
  the various marker tables
@jimporter jimporter force-pushed the jimporter:ipc-markers branch 4 times, most recently from 67421b5 to 26557c8 Oct 21, 2019
@jimporter jimporter force-pushed the jimporter:ipc-markers branch from 26557c8 to 89b02a9 Oct 22, 2019
@jimporter jimporter requested a review from gregtatum Oct 22, 2019
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Oct 22, 2019

@gregtatum Ok, I think this should be good now, at least as an initial version.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 22, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

--HG--
extra : moz-landing-system : lando
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Oct 22, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226
Copy link
Member

gregtatum left a comment

Thanks for working through all of my feedback. This all looks great, and works for me!

@gregtatum gregtatum merged commit c4b64f8 into firefox-devtools:master Oct 22, 2019
9 checks passed
9 checks passed
ci/circleci: alex Your tests passed on CircleCI!
Details
ci/circleci: build-prod Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: licence-check Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details
ci/circleci: yarn_lock Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 23, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: 0fb346412075f87a73de2f98246e636694082171
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 23, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: 0fb346412075f87a73de2f98246e636694082171
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 23, 2019
This adds the ability to add profile markers for both the sender and recipient
sides of IPC messages. These can then be correlated with one another in the
profile visualization. For the UI component of this patch, see
<firefox-devtools/profiler#2172>.

Differential Revision: https://phabricator.services.mozilla.com/D42226

UltraBlame original commit: 0fb346412075f87a73de2f98246e636694082171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.