Closed
Bug 1339695
Opened 7 years ago
Closed 7 years ago
Clean up OS/arch/platform detection in the profiler
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(11 files, 1 obsolete file)
3.42 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
4.74 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
24.91 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
7.22 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.21 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
3.23 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
33.09 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
55.17 KB,
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
10.53 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
OS/arch/platform detection is a mess in the profiler. There is absolutely no consistency about which macros are used. I want to clean this up.
Assignee | ||
Comment 1•7 years ago
|
||
This change increases consistency: - Each OS is dealt with one at a time (no more interleaving). - For each OS, x86 is now the first listed architecture. The patch also adds the missing "#undef SPS_PLAT_x86_android".
Attachment #8837416 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
This factors out some common preprocessor conditions.
Attachment #8837418 -
Flags: review?(jseward)
Assignee | ||
Comment 3•7 years ago
|
||
They duplicate the equivalent SPS_* macros. (The SPS_* macros have already crept into use in some places within LUL.)
Attachment #8837420 -
Flags: review?(jseward)
Assignee | ||
Comment 4•7 years ago
|
||
SPS_STANDALONE was removed in bug 1317771.
Attachment #8837421 -
Flags: review?(mstange)
Assignee | ||
Comment 5•7 years ago
|
||
This factors out some common preprocessor conditions.
Attachment #8837422 -
Flags: review?(mstange)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8837423 -
Flags: review?(mstange)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8837424 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Attachment #8837423 -
Attachment is obsolete: true
Attachment #8837423 -
Flags: review?(mstange)
Comment 8•7 years ago
|
||
Nice tidyup! Yes it was a mess. One comment: do you still want to use the "SPS_" prefix, or should we change that to something else? For some reason I have the impression that it stands for "Simple Profiling System", but now I thought we decided to use the name "Gecko Profiler" for the profiler as a whole.
Updated•7 years ago
|
Attachment #8837418 -
Flags: review?(jseward) → review+
Updated•7 years ago
|
Attachment #8837420 -
Flags: review?(jseward) → review+
Assignee | ||
Comment 9•7 years ago
|
||
Yes, these macros are the only remaining use of SPS left in the code. It would be good to remove them, but it's nice that SPS is such a short prefix. Maybe PROF_* would be good? Or GP_* for "Gecko Profiler"?
Comment 10•7 years ago
|
||
I would vote for GP_, on the basis that PROF_ is a bit too generic.
Updated•7 years ago
|
Attachment #8837416 -
Flags: review?(mstange) → review+
Comment 11•7 years ago
|
||
Comment on attachment 8837421 [details] [diff] [review] (part 4) - Remove a stray, misspelled SPS_STANDALNOE use Review of attachment 8837421 [details] [diff] [review]: ----------------------------------------------------------------- lol
Attachment #8837421 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8837422 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8837424 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9a3692b012221e74ee6fbc70af95dff83e1a7d Bug 1339695 (part 1) - Remove LUL_{ARCH,OS,PLAT}_* macros. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/f6869c7b0707091d551285243c43993d7b235265 Bug 1339695 (part 2) - Reorder PlatformMacros.h.h. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/b735a190b14f0a5ea0f0ccb0793624dd89291055 Bug 1339695 (part 3) - Introduce USE_FAULTY_LIB. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/002e9c45f121f951f205cc3d3ec7fb563f5a831c Bug 1339695 (part 4) - Remove a stray, misspelled SPS_STANDALNOE use. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/6205142186156da59305508759cd08982e38ae1b Bug 1339695 (part 5) - Introduce PROFILE_JAVA. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3b5ef730656f41a31d5b537a83553337c4a549 Bug 1339695 (part 6) - Remove some B2G-only code in profiler_register_thread(). r=mstange.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 13•7 years ago
|
||
It's defined if any of XP_{WIN,MAC,LINUX} are defined and the latter includes Android as well. So it's defined on all the OSes the profiler supports.
Attachment #8837905 -
Flags: review?(mstange)
Assignee | ||
Comment 14•7 years ago
|
||
Currently we use the SPS_* macros in some places, but also use other ones like __arm__ and ANDROID and XP_{WIN,MAC,LINUX}. This patch makes the profiler consistently use the SPS_* macros and removes the V8_HOST_ARCH_* macros. The patch also does the following. - Cleans up some header inclusions, e.g. including pthread.h directly in the files that use it, and removing some unneeded android/log.h inclusions. - Removes an unused branch in SetSampleContext() -- we don't support ARM on anything other than Android, and glibc 2.3 is ancient. - Doesn't use SPS_* in PseudoStack.h because that would require exporting PlatformMacros.h, which doesn't seem worthwhile. Some things that aid the understanding of this patch. - XP_LINUX and LINUX are both defined for Linux *and* Android. - x86/Android is the only supported platform that doesn't define HAVE_NATIVE_UNWIND. - Every platform that defines USE_LUL_STACKWALK also defines HAVE_NATIVE_UNWIND.
Attachment #8837906 -
Flags: review?(mstange)
Assignee | ||
Comment 15•7 years ago
|
||
Specifically: - platform-linux.cc -> platform-linux-android.cpp - platform-macos.cc -> platform-macos.cpp - platform-win32.cc -> platform-win32.cpp Adding "android" to the first one is the most important part, because it makes things clearer. The .cc to .cpp change is less important but I might as well do it while I'm in here.
Attachment #8837907 -
Flags: review?(mstange)
Assignee | ||
Comment 16•7 years ago
|
||
This removes the final mentions of the old "SPS" name.
Attachment #8837908 -
Flags: review?(jseward)
Updated•7 years ago
|
Attachment #8837905 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8837906 -
Flags: review?(mstange) → review+
Comment 17•7 years ago
|
||
Comment on attachment 8837907 [details] [diff] [review] (part 9) - Rename the platform-* files I was wondering when the cc -> cpp change would happen :)
Attachment #8837907 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 18•7 years ago
|
||
We don't need OS now that the platform-*.cpp files are in the same compilation unit as platform.cpp. The patch removes the sleep functions because they are unnecessary indirection. OS::Startup() is necessary, but the patch renames it PlatformInit() to match Platform{Start,Stop}() and profiler_init(), from which it is called.
Attachment #8837928 -
Flags: review?(mstange)
Updated•7 years ago
|
Attachment #8837908 -
Flags: review?(jseward) → review+
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9d9a3692b012 https://hg.mozilla.org/mozilla-central/rev/f6869c7b0707 https://hg.mozilla.org/mozilla-central/rev/b735a190b14f https://hg.mozilla.org/mozilla-central/rev/002e9c45f121 https://hg.mozilla.org/mozilla-central/rev/620514218615 https://hg.mozilla.org/mozilla-central/rev/eb3b5ef73065
Assignee | ||
Comment 20•7 years ago
|
||
mstange: not sure if you overlooked part 11 or if you're just biding your time :)
Comment 21•7 years ago
|
||
Oops, looks like I closed the tab that I had opened for part 11 accidentally and then lost track of it.
Updated•7 years ago
|
Attachment #8837928 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc7d383df21ee79c29dc8ab0942f676569cd1c4 Bug 1339695 (part 7) - Remove ENABLE_LEAF_DATA. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/33c5f636b414db151ddbb6a865878fe53b43e11b Bug 1339695 (part 8) - Clean up platform detection throughout the profiler. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0490c4ada57ffc53da86806021177585ab5343 Bug 1339695 (part 9) - Rename the platform-* files. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/9eed66e9369a5151a26471a0786aad6a41d96fc8 Bug 1339695 (part 10) - Rename SPS_* macros as GP_*. r=jseward. https://hg.mozilla.org/integration/mozilla-inbound/rev/12fae55fe512dfd640b09279821d49b5f9e03a8a Bug 1339695 (part 11) - Remove the profiler's OS class. r=mstange.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8cc7d383df21 https://hg.mozilla.org/mozilla-central/rev/33c5f636b414 https://hg.mozilla.org/mozilla-central/rev/ec0490c4ada5 https://hg.mozilla.org/mozilla-central/rev/9eed66e9369a https://hg.mozilla.org/mozilla-central/rev/12fae55fe512
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•