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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Andrew Creskey [:acreskey]]

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

Comment 2

[Florian Quèze [:florian]]

Nazim, is this something you can help with?

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1616622 - Remove the SparseArray from the samples array since we only profile the main thread. r?julienw,gerald

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.

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1616622 - Rename SamplingThread class to SamplingRunnable instead. r?julienw

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

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1616622 - Increase the buffer limit and refactor the java code r?gerald,julienw

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

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Clearing the needinfo after the patches.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/af4a2ed60003
https://hg.mozilla.org/mozilla-central/rev/43be1e5178c7
https://hg.mozilla.org/mozilla-central/rev/5189b1fee20f