Closed Bug 1721569 Opened 3 years ago Closed 3 years ago

Implement profiler_current_thread_id for all platform, using std::this_thread::get_id() on lower-tier platforms

Categories

(Core :: Gecko Profiler, task, P3)

Product:
Core
Component:
Gecko Profiler
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
93 Branch
Tracking Flags:
Tracking Status
firefox93 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(9 files)

Bug 1721569 - Fix gtest assertions and remove redundant Utils test - r?florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Also compile Gecko Profiler gtest files in non-MOZ_GECKO_PROFILER builds - r?florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Add gtest checks comparing Base and Gecko profiler process/thread APIs - r?florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Handle different sizes of Profiler{Process,Thread}Id - r?florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Make TracePayload easier to modify, and use maximum space for name - r?padenot,florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Moved implementations of {,Base}ProfilerUtils.h declarations to ProfilerUtils.cpp - r?florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Change id native types to reflect what each platform really provides - r?florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Use std::this_thread::get_id() on other platforms - r?florian
48 bytes, text/x-phabricator-request
Details | Review
Bug 1721569 - Move main-thread id functions to cpp files - r?florian
48 bytes, text/x-phabricator-request
Details | Review

Bug 1721110 is hiding the thread id raw type inside a class, which makes it possible to modify it in one place without changing all uses.

std::this_thread::get_id() is actually faster than all our implementations, except on Windows. Micro-benchmarks on Try:

  • Linux 18.04 x64 opt: 130x faster (than syscall)!!!
  • Linux 18.04 x64 WebRender opt: 128x faster (than syscall)!!!
  • Android 7.0 x86-64 opt: 7.4x faster (than gettid)!
  • OS X 10.15 WebRender opt: 2.8x faster (than pthread_threadid_np)
  • Windows 10 x86 WebRender opt: 1.16x slower (than GetCurrentThreadId)
  • Windows 10 x64 WebRender opt: 1.15x slower (than GetCurrentThreadId)

Also, I would assume that <thread> is available on all platforms at all tier levels, so it should work everywhere, and remove the need for checking MOZ_GECKO_PROFILER; this will help with other bits that rely on the thread id (profiler mutex and its users, etc.)

And on Windows, we have another implementation RtlGetCurrentThreadId that is 2-3x faster than GetCurrentThreadId.

I've hit a roadblock, because on Linux (and probably other non-Windows platforms), std::thread::id gives us a large number that looks like a 64-bit pointer (e.g.: 140614794577792 = 0x7fe36ee18780), which has nothing to do with the usual thread ids that are usually < 100,000.
It's probably fine to use in most cases (to compare with other thread ids), but is not very user-friendly to show on profiler.firefox.com, and we still need the "normal" thread ids (from gettid, pthread_threadid_np, or equivalent) in some cases to call system functions that expects them.

I still think bug 1721110 was useful, mainly to give us better type safety.
And it may still be useful to change the underlying types from 32-bit ints to 64-bit numbers on some platforms, so that we don't rely on assumptions about thread ids, e.g., when APIs gives us 64-bit numbers but we assert than they fit in a 32-bit one; this could potentially change.

So I may first implement some follow-up changes (in a separate bug), but will defer using std::thread until I've had a better look at how thread ids are used, and whether the faster speed is worth it.
Lowering priority to P3, as it's not that critical.

Priority: P2 → P3

(In reply to Gerald Squelart [:gerald] (he/him) from comment #1)

I've hit a roadblock, because on Linux (and probably other non-Windows platforms), std::thread::id gives us a large number that looks like a 64-bit pointer (e.g.: 140614794577792 = 0x7fe36ee18780), which has nothing to do with the usual thread ids that are usually < 100,000.
It's probably fine to use in most cases (to compare with other thread ids), but is not very user-friendly to show on profiler.firefox.com, and we still need the "normal" thread ids (from gettid, pthread_threadid_np, or equivalent) in some cases to call system functions that expects them.

That's unfortunate. I wonder if we could still use std::thread::id as a fallback for Tier3 platform that currently don't support MOZ_GECKO_PROFILER (and will never display the values to users).

Ah yes, my thoughts exactly! 😄
It should be just fine on those platforms where we probably won't even take stack samples, but eventually we could have a no-sampling-mode profiler running on them, collecting markers, responsiveness, etc.

But I think for now it will be more valuable for me to continue with the no-lock sampling, and I can revisit this bug here later on...
I'll de-assign myself, in case someone wants to have a look at it in the meantime.

Assignee: gsquelart → nobody

On it again!
But this time I will use std::thread::id only on platforms that don't already have direct support in the profiler.

Assignee: nobody → gsquelart
Summary: Implement profiler_current_thread_id for all platform, using std::this_thread::get_id() in most places → Implement profiler_current_thread_id for all platform, using std::this_thread::get_id() on lower-tier platforms

An upcoming patch will change the size of ProfilerThreadId, which causes troubles with TracePayload.
Trying to move members around didn't help, I'm guessing some padding remains, which cannot be accounted for by just enumerating member sizes.

The proposed solution is to wrap all members in a macro (so they don't need to be repeated), and create a private struct with these members and a character, in order to find the exact offset at which the name will actually start.
This is uglier, but more flexible, and allows future changes (internal or external) more easily, without having to modify the name-size formula.

Depends on D121049

This patch only shuffles source code around, so that all declarations in {,Base}ProfilerUtils.h are now implemented only in ProfilerUtils.cpp (instead of the different platform-*.cpp), the final generated code should be the same in MOZ_GECKO_PROFILER builds (the default on all our supported platforms).
This simplifies the headers and makes further changes easier.

In non-MOZ_GECKO_PROFILER builds: On supported platforms these functions are now fully defined; Unsupported platforms should all had getpid(), but thread ids are null.
So now profiler_current_process_id() is available on all platforms, at all tier levels.

Depends on D121050

Now profiler_current_thread_id() is available on all platforms, at all tier levels.

Depends on D121052

Dan, Landry, Petr: You've reported build issues due to profiler changes in the past. If possible and you have the time, could you please test the attached patches on your favorite systems? (Respectively: Linux/ppc64le and s390x, OpenBSD, Solaris)

This is first to avoid introducing new failures straight away.
But the main goal is to eventually remove the MOZ_GECKO_PROFILER #defines that are currently needed on your systems, so hopefully we will have fewer of these annoying build issues in the future; also parts of the profiler could run there!

[edit] I forgot: After building, please test by running <obj>/dist/bin/TestBaseProfiler, and ./mach gtest *Profiler.*. Thanks!

Flags: needinfo?(petr.sumbera)
Flags: needinfo?(landry)
Flags: needinfo?(dan)

sure, i'll be able to test it as soon as bug #1721707 is fixed :) i suppose the complete patchset is supposed to be tested ?

Flags: needinfo?(landry)

(In reply to Landry Breuil (:gaston) from comment #14)

sure, i'll be able to test it as soon as bug #1721707 is fixed :) i suppose the complete patchset is supposed to be tested ?

Thanks, and yes please: The last patch is probably the most important, as it uses std::thread::id on the "other" platforms and tests it there.

Talking of which: After building, please test by running <obj>/dist/bin/TestBaseProfiler, and ./mach gtest *Profiler.*.

sure, giving it a try now ...

Flags: needinfo?(dan)

Linux/s390x (Fedora 34) looks good - it compiles OK,

[sharkcz@rock-kvmlp-fedora firefox]$ obj-s390x-ibm-linux-gnu/dist/bin/TestBaseProfiler
TestProfilerUtils...
TestProfilerUtils done

 1:58.09 /home/sharkcz/projects/firefox/obj-s390x-ibm-linux-gnu/dist/bin/firefox -unittest --gtest_death_test_style=threadsafe
*** You are running in headless mode.
Running GTest tests...
Note: Google Test filter = *Profiler.*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from GeckoProfiler
[ RUN      ] GeckoProfiler.ProfilerUtils
[       OK ] GeckoProfiler.ProfilerUtils (0 ms)
[----------] 1 test from GeckoProfiler (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 1 test.
Finished running GTest tests.

Linux/ppc64le will follow ...

Linux/ppc64le (Fedora 34) looks good as well, it compiles OK and passes the tests

[dan@talos firefox.git]$ ./obj-powerpc64le-unknown-linux-gnu/dist/bin/TestBaseProfiler
TestProfilerUtils...
TestProfilerUtils done

2:04.95 /mnt/dan/firefox.git/obj-powerpc64le-unknown-linux-gnu/dist/bin/firefox -unittest --gtest_death_test_style=threadsafe
*** You are running in headless mode.
Running GTest tests...
/mozStorageService.cpp:169
/mnt/dan/firefox.git/toolkit/xre/nsUpdateSyncManager.cpp:99
Note: Google Test filter = *Profiler.*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from GeckoProfiler
[ RUN      ] GeckoProfiler.ProfilerUtils
[       OK ] GeckoProfiler.ProfilerUtils (0 ms)
[----------] 1 test from GeckoProfiler (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1 ms total)
[  PASSED  ] 1 test.

With workaround patch for Bug 1721707 I can confirm that your changes build on Solaris. I wasn't able to verify 'mach gtest' as it's probably broken on Solaris.

Flags: needinfo?(petr.sumbera)

(In reply to Petr Sumbera from comment #19)

With workaround patch for Bug 1721707 I can confirm that your changes build on Solaris.

same thing on OpenBSD, m-c builds with your patch stack and D121376. The test seems to fail though ?

[07:52] c64:~/src/m-c/ $uname -a
OpenBSD c64.proxmox2 6.9 GENERIC.MP#149 amd64

[07:53] c64:~/src/m-c/ $/usr/obj/m-c/dist/bin/TestBaseProfiler                                                                                                                                                                                                                   
TestProfilerUtils...
TestProfilerUtils done

[07:55] c64:~/src/m-c/ $MACH_USE_SYSTEM_PYTHON=1 ./mach gtest *Profiler.*
....
One or more Android-only options will be ignored
 2:29.47 /usr/obj/m-c/dist/bin/firefox -unittest --gtest_death_test_style=threadsafe
*** You are running in headless mode.
Running GTest tests...
Note: Google Test filter = *Profiler.*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from GeckoProfiler
[ RUN      ] GeckoProfiler.ProfilerUtils
/home/landry/src/m-c/tools/profiler/tests/gtest/GeckoProfiler.cpp:112: Failure
Expected equality of these values:
  baseprofiler::profiler_main_thread_id()
    Which is: 8-byte object <00-00 00-00 00-00 00-00>
  mainThreadId
    Which is: 8-byte object <28-F4 C4-2D E3-05 00-00>
[  FAILED  ] GeckoProfiler.ProfilerUtils (3 ms)
[----------] 1 test from GeckoProfiler (4 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (5 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] GeckoProfiler.ProfilerUtils

 1 FAILED TEST
Finished running GTest tests.

(In reply to Landry Breuil (:gaston) from comment #20)

same thing on OpenBSD, m-c builds with your patch stack and D121376. The test seems to fail though ?

[ RUN      ] GeckoProfiler.ProfilerUtils
/home/landry/src/m-c/tools/profiler/tests/gtest/GeckoProfiler.cpp:112: Failure
Expected equality of these values:
  baseprofiler::profiler_main_thread_id()
    Which is: 8-byte object <00-00 00-00 00-00 00-00>
  mainThreadId
    Which is: 8-byte object <28-F4 C4-2D E3-05 00-00>

Yes that's definitely a problem. 😢
I've got some ideas, but this will be "fun" to debug. I'll try some things here, but may need your help again...

(In reply to Gerald Squelart [:gerald] (he/him) from comment #21)

(In reply to Landry Breuil (:gaston) from comment #20)

same thing on OpenBSD, m-c builds with your patch stack and D121376. The test seems to fail though ?

[ RUN      ] GeckoProfiler.ProfilerUtils
/home/landry/src/m-c/tools/profiler/tests/gtest/GeckoProfiler.cpp:112: Failure
Expected equality of these values:
  baseprofiler::profiler_main_thread_id()
    Which is: 8-byte object <00-00 00-00 00-00 00-00>
  mainThreadId
    Which is: 8-byte object <28-F4 C4-2D E3-05 00-00>

Yes that's definitely a problem. 😢
I've got some ideas, but this will be "fun" to debug. I'll try some things here, but may need your help again...

no problem, happy to help test things ! sadly in 6 hours i'll be offline until the 15, but throw diffs at me in the meantime and i will try them :) or .. install an openbsd vm ;)

(In reply to Landry Breuil (:gaston) from comment #22)

no problem, happy to help test things ! sadly in 6 hours i'll be offline until the 15, but throw diffs at me in the meantime and i will try them :)

The race is on!
Could you please try this patch: https://hg.mozilla.org/try/rev/3f9b2d17d48d83d696fa4a7489a2b0d09d8a04d0
You may have to rebase the existing patches on a recent central, or grab the rebased patches that are parents of https://hg.mozilla.org/try/rev/3f9b2d17d48d83d696fa4a7489a2b0d09d8a04d0

or .. install an openbsd vm ;)

I've managed to install OpenBSD 6.9 on VMWare, but I'm not sure how to install all the dependencies and build Firefox (yet), I'll certainly try... (I've found an article talking about you! Thank you for maintaining this port.)

(In reply to Gerald Squelart [:gerald] (he/him) from comment #23)

(In reply to Landry Breuil (:gaston) from comment #22)

no problem, happy to help test things ! sadly in 6 hours i'll be offline until the 15, but throw diffs at me in the meantime and i will try them :)

The race is on!
Could you please try this patch: https://hg.mozilla.org/try/rev/3f9b2d17d48d83d696fa4a7489a2b0d09d8a04d0
You may have to rebase the existing patches on a recent central, or grab the rebased patches that are parents of https://hg.mozilla.org/try/rev/3f9b2d17d48d83d696fa4a7489a2b0d09d8a04d0

all the patch stack applied fine on top of central with moz-phab patch D121044 then i tried applying https://hg.mozilla.org/try/raw-rev/3f9b2d17d48d83d696fa4a7489a2b0d09d8a04d0 on top but the last chunk failed:

--------------------------
|diff --git a/tools/profiler/tests/gtest/GeckoProfiler.cpp b/tools/profiler/tests/gtest/GeckoProfiler.cpp
|--- a/tools/profiler/tests/gtest/GeckoProfiler.cpp
|+++ b/tools/profiler/tests/gtest/GeckoProfiler.cpp
--------------------------
Patching file tools/profiler/tests/gtest/GeckoProfiler.cpp using Plan A...
Hunk #1 failed at 48.
Hunk #2 succeeded at 1062 (offset 816 lines).
1 out of 2 hunks failed--saving rejects to tools/profiler/tests/gtest/GeckoProfiler.cpp.rej
Hmm...  Ignoring the trailing garbage.

or .. install an openbsd vm ;)

I've managed to install OpenBSD 6.9 on VMWare, but I'm not sure how to install all the dependencies and build Firefox (yet), I'll certainly try... (I've found an article talking about you! Thank you for maintaining this port.)

as for the dependencies, at some point mach bootstrap had something working for OpenBSD, but i dunno where it stands now ..

pkg_add node cbindgen gtk+3 gm4 autoconf-2.13 gtar unzip zip yasm nasm rust llvm

should get you started with most of the required build dependencies..

i have this locally for .mozconfig:

mk_add_options MOZ_OBJDIR=/usr/obj/m-c
mk_add_options PYTHON=/usr/local/bin/python3.8
mk_add_options AUTOCLOBBER=1
export M4=/usr/local/bin/gm4
export CC=clang
export CXX=clang++
ac_add_options --with-libclang-path=/usr/local/lib

then ./mach build should be fine provided you apply https://phabricator.services.mozilla.com/D121460 first

Thanks for trying.
If you still have the time, here's a one-file patch: https://hg.mozilla.org/try/rev/6361dc7a228f127da53431d1aa3b3d56e784654e

And I'll give my vm a shot, thanks for the information.

(In reply to Gerald Squelart [:gerald] (he/him) from comment #25)

Thanks for trying.
If you still have the time, here's a one-file patch: https://hg.mozilla.org/try/rev/6361dc7a228f127da53431d1aa3b3d56e784654e

sadly mercurial is giving me too much struggle.. is that huge jumbo patch above supposed to be the cumulation of all patches in phabricator ?

https://hg.mozilla.org/try/rev/6361dc7a228f127da53431d1aa3b3d56e784654e is a combination of what's in Phabricator and the extra fixes I'm hoping should work anyway including OpenBSD.

If you're talking about the parent of that ( https://hg.mozilla.org/try/rev/afcdd8e3c67656f49be753e0cf04f18838639543 ), that's mozilla-central.

But I'm guessing it's too late now anyway. Enjoy your holidays!

(In reply to Gerald Squelart [:gerald] (he/him) from comment #27)

https://hg.mozilla.org/try/rev/6361dc7a228f127da53431d1aa3b3d56e784654e is a combination of what's in Phabricator and the extra fixes I'm hoping should work anyway including OpenBSD.

If you're talking about the parent of that ( https://hg.mozilla.org/try/rev/afcdd8e3c67656f49be753e0cf04f18838639543 ), that's mozilla-central.

But I'm guessing it's too late now anyway. Enjoy your holidays!

https://hg.mozilla.org/try/rev/6361dc7a228f127da53431d1aa3b3d56e784654e applied on top of m-c with https://phabricator.services.mozilla.com/D121460 just below, hopefully my build will finish before i leave ;)

<prof' farnsworth voice>Good news everyone!</prof>

[17:00] c64:~/src/m-c/ $MACH_USE_SYSTEM_PYTHON=1 ./mach gtest *Profiler.*
...
 1:39.52 /usr/obj/m-c/dist/bin/firefox -unittest --gtest_death_test_style=threadsafe
*** You are running in headless mode.
Running GTest tests...
Note: Google Test filter = *Profiler.*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from GeckoProfiler
[ RUN      ] GeckoProfiler.ProfilerUtils
[       OK ] GeckoProfiler.ProfilerUtils (1 ms)
[----------] 1 test from GeckoProfiler (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (3 ms total)
[  PASSED  ] 1 test.
Finished running GTest tests.

so this jumbo patch works on OpenBSD/amd64. Thanks !

Great, thank you Landry, and have a good time off. Also thanks Dan and Petr.

I will rework the patchset to include these changes, I think they make for better code anyway.

Attachment #9233446 - Attachment description: Bug 1721569 - Handle different sizes of Profiler{Process,Thread}Id::NumberType - r?florian → Bug 1721569 - Handle different sizes of Profiler{Process,Thread}Id - r?florian

This hides the scProfilerMainThreadId detail, and makes for a safer API.
Also, ::profiler_init_main_thread_id() calls ::mozilla::baseprofiler::profiler_init_main_thread_id().
And in non-MOZ_GECKO_PROFILER builds, AUTO_PROFILER_INIT calls profiler_init_main_thread_id(), which makes other main-thread functions usable there (assuming profiler_current_thread_id works).

Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9fb7b238251b Fix gtest assertions and remove redundant Utils test - r=florian https://hg.mozilla.org/integration/autoland/rev/f6709a3b2bba Also compile Gecko Profiler gtest files in non-MOZ_GECKO_PROFILER builds - r=florian https://hg.mozilla.org/integration/autoland/rev/c1effaec69ba Add gtest checks comparing Base and Gecko profiler process/thread APIs - r=florian https://hg.mozilla.org/integration/autoland/rev/e3d5648f410e Handle different sizes of Profiler{Process,Thread}Id - r=florian https://hg.mozilla.org/integration/autoland/rev/92c07c54be8a Make TracePayload easier to modify, and use maximum space for name - r=padenot https://hg.mozilla.org/integration/autoland/rev/628ecb915610 Moved implementations of {,Base}ProfilerUtils.h declarations to ProfilerUtils.cpp - r=florian https://hg.mozilla.org/integration/autoland/rev/a62b994ad65b Move main-thread id functions to cpp files - r=florian https://hg.mozilla.org/integration/autoland/rev/e613ebfbbd2d Change id native types to reflect what each platform really provides - r=florian https://hg.mozilla.org/integration/autoland/rev/f88160d39ad0 Use std::this_thread::get_id() on other platforms - r=florian
See Also: → 1841030
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: