Clean up OS/arch/platform detection in the profiler

RESOLVED FIXED in Firefox 54

Status

()

Core
Gecko Profiler
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(11 attachments, 1 obsolete attachment)

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
(Assignee)

Description

6 months ago
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

6 months ago
Created attachment 8837416 [details] [diff] [review]
(part 2) - Reorder PlatformMacros.h.h

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

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

Comment 2

6 months ago
Created attachment 8837418 [details] [diff] [review]
(part 3) - Introduce USE_FAULTY_LIB

This factors out some common preprocessor conditions.
Attachment #8837418 - Flags: review?(jseward)
(Assignee)

Comment 3

6 months ago
Created attachment 8837420 [details] [diff] [review]
(part 1) - Remove LUL_{ARCH,OS,PLAT}_* macros

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

6 months ago
Created attachment 8837421 [details] [diff] [review]
(part 4) - Remove a stray, misspelled SPS_STANDALNOE use

SPS_STANDALONE was removed in bug 1317771.
Attachment #8837421 - Flags: review?(mstange)
(Assignee)

Comment 5

6 months ago
Created attachment 8837422 [details] [diff] [review]
(part 5) - Introduce PROFILE_JAVA

This factors out some common preprocessor conditions.
Attachment #8837422 - Flags: review?(mstange)
(Assignee)

Comment 6

6 months ago
Created attachment 8837423 [details] [diff] [review]
(part 6) - Remove some B2G-only code in profiler_register_thread()
Attachment #8837423 - Flags: review?(mstange)
(Assignee)

Comment 7

6 months ago
Created attachment 8837424 [details] [diff] [review]
(part 6) - Remove some B2G-only code in profiler_register_thread()
Attachment #8837424 - Flags: review?(mstange)
(Assignee)

Updated

6 months ago
Attachment #8837423 - Attachment is obsolete: true
Attachment #8837423 - Flags: review?(mstange)
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.
Attachment #8837418 - Flags: review?(jseward) → review+
Attachment #8837420 - Flags: review?(jseward) → review+
(Assignee)

Comment 9

6 months 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"?
I would vote for GP_, on the basis that PROF_ is a bit too generic.
Attachment #8837416 - Flags: review?(mstange) → review+
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+
Attachment #8837422 - Flags: review?(mstange) → review+
Attachment #8837424 - Flags: review?(mstange) → review+
(Assignee)

Comment 12

6 months 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

6 months ago
Keywords: leave-open
(Assignee)

Comment 13

6 months ago
Created attachment 8837905 [details] [diff] [review]
(part 7) - Remove ENABLE_LEAF_DATA

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

6 months ago
Created attachment 8837906 [details] [diff] [review]
(part 8) - Clean up platform detection throughout the profiler

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

6 months ago
Created attachment 8837907 [details] [diff] [review]
(part 9) - Rename the platform-* files

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

6 months ago
Created attachment 8837908 [details] [diff] [review]
(part 10) - Rename SPS_* macros as GP_*

This removes the final mentions of the old "SPS" name.
Attachment #8837908 - Flags: review?(jseward)
Attachment #8837905 - Flags: review?(mstange) → review+
Attachment #8837906 - Flags: review?(mstange) → review+
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

6 months ago
Created attachment 8837928 [details] [diff] [review]
(part 11) - Remove the profiler's OS class

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)
Attachment #8837908 - Flags: review?(jseward) → review+

Comment 19

6 months 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

6 months ago
mstange: not sure if you overlooked part 11 or if you're just biding your time :)
Oops, looks like I closed the tab that I had opened for part 11 accidentally and then lost track of it.
Attachment #8837928 - Flags: review?(mstange) → review+
(Assignee)

Comment 22

6 months 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

6 months ago
Keywords: leave-open

Comment 23

6 months 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
Last Resolved: 6 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

5 months ago
Depends on: 1350211
Depends on: 1348776
You need to log in before you can comment on or make changes to this bug.