Closed Bug 1449983 Opened 2 years ago Closed 2 years ago

The "Java" thread has the wrong JSON format

Categories

(Core :: Gecko Profiler, enhancement, P1)

enhancement

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.
Priority: -- → P1
Assignee: nobody → m_kato
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+
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.
(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?
(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.
Sample count of GeckoJavaSampler is always 1000.
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/4a75b60fccb6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1460973
You need to log in before you can comment on or make changes to this bug.