Closed
Bug 1449983
Opened 6 years ago
Closed 6 years ago
The "Java" thread has the wrong JSON format
Categories
(Core :: Gecko Profiler, enhancement, P1)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: mstange, Assigned: m_kato)
References
Details
Attachments
(1 file)
When you specify the "java" feature during profiling on Android, we generate a separate thread with Java callstacks. This is done in a function called BuildJavaThreadJSObject at https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/tools/profiler/core/platform.cpp#1670-1720 . However, this code was not updated when the JSON format was updated a few years ago, and the JSON it generates cannot be parsed by perf.html. We need to update this function to produce JSON with the new format.
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → m_kato
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbaad787a2851243051c54ed8028e0187c90500b
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8969519 [details] Bug 1449983 - The Java thread has the wrong JSON format. https://reviewboard.mozilla.org/r/238280/#review244482 ::: tools/profiler/core/platform.cpp:1748 (Diff revision 1) > > #if defined(GP_OS_android) > - if (ActivePS::FeatureJava(aLock)) { > + if (ActivePS::FeatureJava(aLock)) { > - java::GeckoJavaSampler::Pause(); > + java::GeckoJavaSampler::Pause(); > > - aWriter.Start(); > + CollectJavaThreadProfileData(buffer); So this adds the Java thread's data into the profiler buffer... and the data will stay there. If you grab another profile after this, will there be duplicate data in the buffer? Maybe it would be better to create a new, short-lived ProfilerBuffer here. But then you'd have to estimate its size... maybe it's ok the way you have it here. ::: tools/profiler/core/platform.cpp:1751 (Diff revision 1) > - java::GeckoJavaSampler::Pause(); > + java::GeckoJavaSampler::Pause(); > > - aWriter.Start(); > - { > - BuildJavaThreadJSObject(aWriter); > - } > + CollectJavaThreadProfileData(buffer); > + > + RefPtr<ThreadInfo> threadInfo = > + new ThreadInfo("Java Main Thread", 0, false); I think here you're relying on the assumption that no other thread is going to have thread ID 0. Otherwise the call to StreamJSON would also add the other thread's samples. I think that assumption is fine, but you should add a comment about it.
Attachment #8969519 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969519 [details] Bug 1449983 - The Java thread has the wrong JSON format. https://reviewboard.mozilla.org/r/238280/#review244482 > So this adds the Java thread's data into the profiler buffer... and the data will stay there. If you grab another profile after this, will there be duplicate data in the buffer? > > Maybe it would be better to create a new, short-lived ProfilerBuffer here. But then you'd have to estimate its size... maybe it's ok the way you have it here. Java thread isn't profiled from C++, so it isn't duplicated. And, sampling of java profiler is 10ms as min. So profiling data keeps small. I will file a follow up bug to create new profiler buffer. > I think here you're relying on the assumption that no other thread is going to have thread ID 0. Otherwise the call to StreamJSON would also add the other thread's samples. > > I think that assumption is fine, but you should add a comment about it. OK, I will add it.
Reporter | ||
Comment 5•6 years ago
|
||
(In reply to Makoto Kato [:m_kato] from comment #4) > Comment on attachment 8969519 [details] > Bug 1449983 - The Java thread has the wrong JSON format. > > https://reviewboard.mozilla.org/r/238280/#review244482 > > > So this adds the Java thread's data into the profiler buffer... and the data will stay there. If you grab another profile after this, will there be duplicate data in the buffer? > > > > Maybe it would be better to create a new, short-lived ProfilerBuffer here. But then you'd have to estimate its size... maybe it's ok the way you have it here. > > Java thread isn't profiled from C++, so it isn't duplicated. What I meant is: 1. The profiler is started. 2. At some point, the JSON is dumped. At this point, the buffer contains samples + stacks from C++. 3. You add the JSON stacks to the buffer. 4. JSON dumping is finished and the profiler keeps running. 5. At some point, the JSON is dumped again. At this point, the buffer contains samples from C++, then some samples from Java, and then more samples from C++. 6. Now you add more samples from Java. Can java::GeckoJavaSampler::GetSampleTime / java::GeckoJavaSampler::GetFrameName return data that they've returned before? Or are the collected Java stacks reset?
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #5) > What I meant is: > > 1. The profiler is started. > 2. At some point, the JSON is dumped. At this point, the buffer contains > samples + stacks from C++. > 3. You add the JSON stacks to the buffer. > 4. JSON dumping is finished and the profiler keeps running. > 5. At some point, the JSON is dumped again. At this point, the buffer > contains samples from C++, then some samples from Java, and then more > samples from C++. > 6. Now you add more samples from Java. > > Can java::GeckoJavaSampler::GetSampleTime / > java::GeckoJavaSampler::GetFrameName return data that they've returned > before? Or are the collected Java stacks reset? Ah, OK. Java's sampling isn't destroyed when dumping only. It is destroyed by stop method. (New remote profiler UI implementation has only dumping and stop, so this UX destroys it by showing profile). So I should add local scoped ProfileBuffer.
Assignee | ||
Comment 7•6 years ago
|
||
Sample count of GeckoJavaSampler is always 1000.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 8969519 [details] Bug 1449983 - The Java thread has the wrong JSON format. Update to use separated ProfileBuffer.
Attachment #8969519 -
Flags: review+ → review?(mstange)
Reporter | ||
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8969519 [details] Bug 1449983 - The Java thread has the wrong JSON format. https://reviewboard.mozilla.org/r/238280/#review245210
Attachment #8969519 -
Flags: review?(mstange) → review+
Comment 11•6 years ago
|
||
Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/autoland/rev/4a75b60fccb6 The Java thread has the wrong JSON format. r=mstange
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a75b60fccb6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•