Closed
Bug 960153
Opened 10 years ago
Closed 7 years ago
Allow specifying thread filters and profiler features for startup profiling with an env variable
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: mstange, Assigned: mstange)
References
Details
Attachments
(3 files, 1 obsolete file)
No description provided.
Attachment #8360504 -
Flags: review?(bgirard)
Comment 1•10 years ago
|
||
Comment on attachment 8360504 [details] [diff] [review] v1 Review of attachment 8360504 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I was bit distracted on IRC I should of given you a better answer. We should probably do something similar to ReadProfilerVars here rather then rewriting this code.
Comment 2•10 years ago
|
||
Comment on attachment 8360504 [details] [diff] [review] v1 Waiting on conversation before reviewing this.
Attachment #8360504 -
Flags: review?(bgirard)
Assignee | ||
Comment 3•7 years ago
|
||
Morphing this bug to cover profiler features as well.
Summary: Add an environment variable for selecting profiled threads, MOZ_PROFILER_THREADS → Allow specifying thread filters and profiler features for startup profiling with an env variable
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8888973 [details] Bug 960153 - Add env var MOZ_PROFILER_STARTUP_FILTERS that lets you select which threads should be profiled during startup profiling. https://reviewboard.mozilla.org/r/159992/#review165428 ::: tools/profiler/core/platform.cpp:2080 (Diff revision 1) > + nsTArray<const char*> array; > + size_t len = strlen(aString); > + size_t currentElementStart = 0; > + aStorage = MakeUnique<char[]>(len + 1); > + // Iterate over all characters and split at commas. > + for (size_t i = 0; i < len; i++) { This works but I suspect it would be simpler if you strdup'd |aString| and then iterated over the copy, converting the commas to '\0' and recording the pointers in |array| as you go. ::: tools/profiler/core/platform.cpp:2105 (Diff revision 1) > > SharedLibraryInfo::Initialize(); > > + if (getenv("MOZ_PROFILER_HELP")) { > + PrintUsageThenExit(0); // terminates execution > + } You could move this above SharedLibraryInfo::Initialize().
Attachment #8888973 -
Flags: review?(n.nethercote) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8888974 [details] Bug 960153 - Factor out ParseFeaturesFromStringArray. https://reviewboard.mozilla.org/r/159994/#review165430 You sure you don't want to change nsIProfiler and the profiler extension to use a bitfield instead of this list-of-strings approach? :)
Attachment #8888974 -
Flags: review?(n.nethercote) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8888975 [details] Bug 960153 - Add env var MOZ_PROFILER_STARTUP_FEATURES that lets you select which features should be active for startup profiling. https://reviewboard.mozilla.org/r/159996/#review165432
Attachment #8888975 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8360504 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8888973 [details] Bug 960153 - Add env var MOZ_PROFILER_STARTUP_FILTERS that lets you select which threads should be profiled during startup profiling. https://reviewboard.mozilla.org/r/159992/#review165428 > This works but I suspect it would be simpler if you strdup'd |aString| and then iterated over the copy, converting the commas to '\0' and recording the pointers in |array| as you go. I'm now using MakeUnique and PodCopy, and then iterate over the copied string. I didn't want to use strdup because then I would have had to use a UniquePtr<char,JS::FreePolicy> and somehow that seemed too complicated to me. > You could move this above SharedLibraryInfo::Initialize(). Done.
Comment 14•7 years ago
|
||
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/50ef560f1f56 Add env var MOZ_PROFILER_STARTUP_FILTERS that lets you select which threads should be profiled during startup profiling. r=njn https://hg.mozilla.org/integration/autoland/rev/4ef452538efd Factor out ParseFeaturesFromStringArray. r=njn https://hg.mozilla.org/integration/autoland/rev/4b4ef581b92a Add env var MOZ_PROFILER_STARTUP_FEATURES that lets you select which features should be active for startup profiling. r=njn
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50ef560f1f56 https://hg.mozilla.org/mozilla-central/rev/4ef452538efd https://hg.mozilla.org/mozilla-central/rev/4b4ef581b92a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 16•7 years ago
|
||
Just putting this on your radar florian, since I recall you needing this capability.
You need to log in
before you can comment on or make changes to this bug.
Description
•