Closed Bug 1616622 Opened 5 months ago Closed 4 months ago

Java sampling is limited to 10 seconds due to a hardcoded limit

Categories

(Core :: Gecko Profiler, defect, P2)

All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: mstange, Assigned: canova)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Profile: https://perfht.ml/2wxgTw3

It would be nice to capture more than 10 seconds of Java sampling. At the moment, there's an artificial limit to 1000 samples:
https://searchfox.org/mozilla-central/rev/fca0be7e2cf2f922c9b927423ce28e8a04b3fd90/tools/profiler/core/platform.cpp#4200

The Java profiling code hasn't seen much love and is basically unchanged from its state in 2013.

Since Fenix makes heavy use of Java/Kotlin, I think Java profiling is more important than it once was.

Nazim, is this something you can help with?

Flags: needinfo?(canaltinova)
Priority: -- → P2
Depends on: 1616887
Blocks: 1616887
No longer depends on: 1616887

Sorry about not writing here before, I saw that and looking into it. Keeping the needinfo.

Assignee: nobody → canaltinova
Status: NEW → ASSIGNED

Currently we only profile the Java Main Thread, and don't profile anything
else. This is not ideal, but this is how it works right now. And inside the
code index 0 was hardcoded on the most parts of the code. We can rollback
this patch once we want to implement profiling more than one thread, or we can
think about something more clever.

This class is not technically the thread itself, it's the runnable that the
sampling thread uses. This name is more accurate and clear for it.

Depends on D64752

The previous samples buffer had a limited size and it was only keeping the
samples for 10 seconds. We wanted to increase the amount of samples we want to
keep but we also wanted to make sure that we are not allocating a lot of memory
at the start. So we created a segmented buffer instead(LinkedList<Sample[]>)
With this buffer, we don't create the whole buffer at the start but we create
small portions of the buffer on the go. Since we use LinkedList, we do not copy
all the memory while allocating more memory, so it's more performant compared
to its alternative.
We also had to keep the circular nature of the buffer and make sure that we are
not breaking java side when we are out of the whole segmented buffer.
Disclaimer: This circular buffer is extremely naive and doesn't look like the
circular buffer in the profiler cpp side. But I wanted to keep it like this
because 1) it was like this in it's previous implementation. 2) I wanted to
land this work as soon as possible so people can start profiling java thread
more than 10 seconds sooner. But I intend to work on this after this work and
make it more performant.

Depends on D64753

Clearing the needinfo after the patches.

Flags: needinfo?(canaltinova)
Attachment #9129768 - Attachment description: Bug 1616622 - Implement a segmented circular buffer and increase the buffer limit r?gerald,julienw → Bug 1616622 - Increase the buffer limit and refactor the java code r?gerald,julienw
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/af4a2ed60003
Remove the SparseArray from the samples array since we only profile the main thread. r=julienw
https://hg.mozilla.org/integration/autoland/rev/43be1e5178c7
Rename SamplingThread class to SamplingRunnable instead. r=julienw
https://hg.mozilla.org/integration/autoland/rev/5189b1fee20f
Increase the buffer limit and refactor the java code r=gerald,julienw
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.