Closed Bug 1641122 Opened 10 months ago Closed 10 months ago

Default value for MOZ_PROFILER_STARTUP_ENTRIES is too high for Android 32 bit devices

Categories

(Core :: Gecko Profiler, defect, P2)

Product:
Core
Component:
Gecko Profiler
Platform:
ARM
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Tracking Flags:
Tracking Status
firefox78 --- fixed

People

(Reporter: mstange, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1641122 - Fix 32-bit Android profiler buffer size default - r?canaltinova
47 bytes, text/x-phabricator-request
Details | Review

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.

Would you have recommendations for these defaults? (both #ifdef's and values.)

Severity: -- → S3
Flags: needinfo?(mstange)
Priority: -- → P2

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:

So maybe GP_PLAT_arm_android and 2 * 1024 * 1024.

Flags: needinfo?(mstange)

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.

Thank you Markus, very useful!

Assignee: nobody → gsquelart

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?

Flags: needinfo?(mstange)

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.

Flags: needinfo?(mstange)
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03b1fa7ea69e
Fix 32-bit Android profiler buffer size default - r=canaltinova

I'm still pushing the update, mainly because of the incorrect #ifdef ARCH_ARMV6, and to increase the defaults a bit.

https://hg.mozilla.org/mozilla-central/rev/03b1fa7ea69e

Status: NEW → RESOLVED
Closed: 10 months ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.