A bunch more Profiler clean-ups

RESOLVED FIXED in Firefox 54

Status

()

Core
Gecko Profiler
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(8 attachments)

(Assignee)

Description

2 months ago
I have a bunch more profiler clean-ups.
(Assignee)

Comment 1

2 months ago
Created attachment 8836991 [details] [diff] [review]
(part 1) - Fix profiler_get_features()

"unwinder" and "jank" are never checked for via hasFeature(), and are not
mentioned anywhere else in the profiler. This patch removes them from
profiler_get_features().

"restyle" *does* have a hasFeature() call and is handled by
profiler_feature_active(). This patch adds it to profiler_get_features().
Attachment #8836991 - Flags: review?(mstange)
(Assignee)

Updated

2 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 months ago
Created attachment 8836992 [details] [diff] [review]
(part 2) - Tweak platform-*.cc files

This patch:

- Improves some comments.

- Adds some assertions.

- Removes unnecessary differences between some Mac and Windows code.

- Removes the unnecessary SamplerThread::mStackSize on Windows.
Attachment #8836992 - Flags: review?(mstange)
(Assignee)

Comment 3

2 months ago
Created attachment 8836993 [details] [diff] [review]
(part 3) - Remove ToStreamAsJSON()

This is possible because profiler_shutdown() can call
profiler_save_profile_to_file() to do the saving.
Attachment #8836993 - Flags: review?(mstange)
(Assignee)

Comment 4

2 months ago
Created attachment 8836994 [details] [diff] [review]
(part 4) - Rename some globals in the profiler

The 'g' prefix is for global variables. The 's' prefix is for static class
members.
Attachment #8836994 - Flags: review?(mstange)
(Assignee)

Comment 5

2 months ago
Created attachment 8836995 [details] [diff] [review]
(part 5) - Rename some profiler functions related to sleeping

The new names make it clearer that these actions apply to just one thread.

- profiler_sleep_start() --> profiler_thread_sleep()
- profiler_sleep_end()   --> profiler_thread_wake()
- profiler_is_sleeping() --> profiler_thread_is_sleeping()
- GeckoProfilerSleepRAII --> GeckoProfilerThreadSleepRAII
- GeckoProfilerWakeRAII  --> GeckoProfilerThreadWakeRAII
Attachment #8836995 - Flags: review?(mstange)
(Assignee)

Comment 6

2 months ago
Created attachment 8836997 [details] [diff] [review]
(part 6) - Remove v8-support.h

It's entirely unused, except for DISALLOW_COPY_AND_ASSIGN, which is easy to
replace.
Attachment #8836997 - Flags: review?(mstange)
(Assignee)

Comment 7

2 months ago
Created attachment 8836998 [details] [diff] [review]
(part 7) - Rename some things in platform-linux.cc

The new names are more consistent, and I find them clearer.

The patch also inlines and removes the horribly-named ProfilerSignalThread()
function.
Attachment #8836998 - Flags: review?(mstange)
(Assignee)

Comment 8

2 months ago
Created attachment 8837000 [details] [diff] [review]
(part 8) - Move the LUL printing check out of SignalSender()'s inner loop

Instead of calling MaybeShowStats() every 16th time around the inner loop we
now check every time around the outer loop. Because there are typically 20--50
threads running at once, this results in a slightly lower freqency of printing,
but that seems fine because this is debug-only code.
Attachment #8837000 - Flags: review?(jseward)
Attachment #8837000 - Flags: review?(jseward) → review+
Attachment #8836991 - Flags: review?(mstange) → review+
Attachment #8836992 - Flags: review?(mstange) → review+
Attachment #8836993 - Flags: review?(mstange) → review+
Attachment #8836994 - Flags: review?(mstange) → review+
Attachment #8836995 - Flags: review?(mstange) → review+
Attachment #8836997 - Flags: review?(mstange) → review+
Attachment #8836998 - Flags: review?(mstange) → review+
(Assignee)

Comment 9

2 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5da29c4aafb01e308dde13260adc8edf1ab2bf65
Bug 1339327 (part 1) - Fix profiler_get_features(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c0da509e47dcd9907badd5c9fde1a3356f95f
Bug 1339327 (part 2) - Tweak platform-*.cc files. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb6c0a02b7e491cafef68ba782791ebdde8b878
Bug 1339327 (part 3) - Remove ToStreamAsJSON(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6933c5687aaee7f3c261364f077b4d1fa4d3d8a
Bug 1339327 (part 4) - Rename some globals in the profiler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa60b9dbadcc1352193a4b64367ed2135dd384d
Bug 1339327 (part 5) - Rename some profiler functions related to sleeping. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c8118d95f32d06261d69317e3d14f1b4fd9648
Bug 1339327 (part 6) - Remove v8-support.h. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/542fffc2c125254f39a8457abfb69671269c6dd1
Bug 1339327 (part 7) - Rename some things in platform-linux.cc. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e538c3740be131f78a98129221f16d4dacb29906
Bug 1339327 (part 8) - Move the LUL printing check out of SignalSender()'s inner loop. r=jseward.

Comment 10

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5da29c4aafb0
https://hg.mozilla.org/mozilla-central/rev/bc1c0da509e4
https://hg.mozilla.org/mozilla-central/rev/5cb6c0a02b7e
https://hg.mozilla.org/mozilla-central/rev/c6933c5687aa
https://hg.mozilla.org/mozilla-central/rev/1aa60b9dbadc
https://hg.mozilla.org/mozilla-central/rev/d6c8118d95f3
https://hg.mozilla.org/mozilla-central/rev/542fffc2c125
https://hg.mozilla.org/mozilla-central/rev/e538c3740be1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.