A bunch more Profiler clean-ups

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(8 attachments)

Assignee

Description

2 years ago
I have a bunch more profiler clean-ups.
Assignee

Comment 1

2 years ago
"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 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee

Comment 2

2 years ago
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 years ago
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 years ago
The 'g' prefix is for global variables. The 's' prefix is for static class
members.
Attachment #8836994 - Flags: review?(mstange)
Assignee

Comment 5

2 years ago
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 years ago
It's entirely unused, except for DISALLOW_COPY_AND_ASSIGN, which is easy to
replace.
Attachment #8836997 - Flags: review?(mstange)
Assignee

Comment 7

2 years ago
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 years ago
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+
You need to log in before you can comment on or make changes to this bug.