Default value for MOZ_PROFILER_STARTUP_ENTRIES is too high for Android 32 bit devices
Categories
(Core :: Gecko Profiler, defect, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox78 | --- | fixed |
People
(Reporter: mstange, Assigned: gerald)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
With the default PROFILER_DEFAULT_STARTUP_ENTRIES, startup profiling on a Moto G5 now causes a startup crash, see bug 1641119. It seems that the default value for PROFILER_DEFAULT_STARTUP_ENTRIES is too high for that device, at least when the Java feature is enabled and we're doing another buffer allocation on the Java side.
We currently have the following in the code:
static constexpr mozilla::PowerOfTwo32 PROFILER_DEFAULT_STARTUP_ENTRIES =
# if !defined(ARCH_ARMV6)
mozilla::MakePowerOfTwo32<64 * 1024 * 1024>(); // 64M entries = 512MB
# else
mozilla::MakePowerOfTwo32<512 * 1024>(); // 512k entries = 4MB
# endif
My Android 32 bit builds are getting the 512MB value as a default, so I'm pretty sure ARCH_ARMV6 is no longer set and we're using 512MB everywhere now. (The ARCH_ARMV6 limit was introduced in bug 802333.)
It may be time to tweak these defines and pick new values.
|
Assignee | |
Comment 1•4 months ago
|
||
Would you have recommendations for these defaults? (both #ifdef's and values.)
|
Reporter | |
Comment 2•4 months ago
|
||
I experimented with a few values. The buffer-size-per-time consumption is very different between the Java thread and the regular threads; the Java thread needs about 10x fewer entries to capture the same period of time. I found a value of 1600000 to be sufficient for all interesting threads during app link startup (~15 seconds). A small value of 100000 was enough to capture Java data for the entire duration of startup, but way too small for C++ data.
Profiles:
- 100000 (128KB): https://share.firefox.dev/2A6ltmF
- 200000 (256KB): https://share.firefox.dev/3c2sX7r
- 400000 (512KB): https://share.firefox.dev/3c6cPBK
- 800000 (1MB): https://share.firefox.dev/3c56Ihh
|
|
Reporter | |
Comment 3•4 months ago
|
||
So maybe GP_PLAT_arm_android and 2 * 1024 * 1024.
|
|
Reporter | |
Comment 4•4 months ago
|
||
Could probably even go much higher; the OOM was on the Java side, not on the C++ side, so I think something about the Java side's interpretation of these capacity values has changed. Maybe we can tweak that side.
It looks like this was because Bug 1639630. We were using the same size with the C++ buffer due to a simple typo, so if you set like a 256mb as the buffer size, it was also trying to allocate 256mb for Java despite having a static cap for the Java buffer. It should be fixed 4 days ago, Markus, can you still reproduce it with the latest mozilla-central?
|
|
Assignee | |
Comment 7•4 months ago
|
||
|
|
Reporter | |
Comment 8•4 months ago
|
||
After updating today I can no longer reproduce. I was using the most recent Fenix build as of yesterday, but it was on an old Gecko.
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/03b1fa7ea69e Fix 32-bit Android profiler buffer size default - r=canaltinova
|
|
Assignee | |
Comment 10•4 months ago
|
||
I'm still pushing the update, mainly because of the incorrect #ifdef ARCH_ARMV6, and to increase the defaults a bit.
|
||
Comment 11•4 months ago
|
||
| bugherder | ||
Description
•