Closed Bug 1339695 Opened 3 years ago Closed 3 years ago

Clean up OS/arch/platform detection in the profiler

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: njn, Assigned: njn)

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.
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: nobody → n.nethercote
Status: NEW → ASSIGNED
This factors out some common preprocessor conditions.
Attachment #8837418 - Flags: review?(jseward)
They duplicate the equivalent SPS_* macros. (The SPS_* macros have already
crept into use in some places within LUL.)
Attachment #8837420 - Flags: review?(jseward)
This factors out some common preprocessor conditions.
Attachment #8837422 - Flags: review?(mstange)
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+
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+
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)
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)
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)
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+
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+
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+
You need to log in before you can comment on or make changes to this bug.