867728 - Stream profiler JSON directly to a file

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Right now the profiler can save a profile to a JSON string but needs to build a full memory representation of the profile. We need to refactor this code to stream the JSON directly to the file. We'd need to modify the JSON builder to track of the state and the pending close tags but it shouldn't be a ton of work.

Comment 3

[Viktor Stanchev [:vikstrous]]

This turned out to be more difficult than we thought initially.

The current API for creating JSON objects is generally used like this:

Create object A
Create object B
Add abject B under Object A for key "b"

We always create the object before we know where in the tree it will go. I can't think of a non-hacky solution to this issue without changing the API.

The JSON building code right now is incredibly complex for the little work it needs to do in the streaming case. I would much rather replace all uses of JSON serialization in the profiler than add another complex (and fragile) hack on top of this.

I'm working on a solution that has the same API as the android JsonWriter: https://developer.android.com/reference/android/util/JsonWriter.html

The diff will be large, but I think it will be a net decrease in the lines of code and complexity. There is only one place that needs the actual JSON and that's the getProfileData call from the browser. For simplicity, in that case, I think it makes sense to just use our json streaming code to write to a sting stream and then run JSON.parse() under the hood. That's used only on desktop right? The memory use will be increased in that case, but I think it's a good trade off.

Also, do we even need that API to return an object? It looks like the profiler addon adds a few fields to it and then just stingifies it anyway. We could just pass whatever it's adding to the profile separately and have a more efficient profiler API from javascript that just returns the stringified profile.

Comment 4

[Viktor Stanchev [:vikstrous]]

One thing that might interfere with streaming profiles might be ProfileSaveEvent. It seems to emit an event and let something add additional threads to the profile, but I don't really understand how it's being used. I'll investigate when I get there...

Comment 5

[Viktor Stanchev [:vikstrous]]

While working on this patch I noticed that every sample has a "name" but it doesn't seem to be used anywhere, so I think I should remove it. Removing it makes the code much simpler. Let me know if there was a reason for it.

Comment 6

[Benoit Girard (:BenWa)]

For this patch I think that having a stringify API only is fine. It will let us stream and desktop can suffer some performance overhead is need be. Looks like you have a good plan. This will make the code much better!

If the name field is never used then I agree that we should remove it.

Comment 7

[Viktor Stanchev [:vikstrous]]

Things are going well so far. My patch's current stats are around +650 -1000 lines if I don't count the tests. I was a bit uncertain about how the plugin profiling works, but I think I haven't broken anything. How can I test it? Do we use it right now? Do you have an example profile that has plugins?

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Viktor Stanchev [:vikstrous] from comment #7)
> Things are going well so far. My patch's current stats are around +650 -1000
> lines if I don't count the tests. I was a bit uncertain about how the plugin
> profiling works, but I think I haven't broken anything. How can I test it?
> Do we use it right now? Do you have an example profile that has plugins?

We can regress the plugin part of this patch since it's not widely used. We can fix it once it's needed again.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Note that for the devtools profiler we plan to use the bulk data transport of the remote protocol when it's ready (bug 797639). I don't know how this work affects that one, so I'm CCing the experts.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

With bulk data, we'd be able to take any random nsIInputStream and stream it across the Dev Tools protocol's socket, as opposed to serializing tons of JSON as we do now.  So, if this change would make it possible to put the data into such a stream, that sounds great!

I don't know much about profiler internals...  It's not clear to me whether this change would bring us closer to or further from an input stream we could use with the Dev Tools.

Comment 11

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json

I've tested this patch on b2g, androind and linux and just sent it to the try server. https://tbpl.mozilla.org/?tree=Try&rev=52b55a53a4c4 (disregard all the individual commits)

On b2g I was able to get profiles in the 10s of seconds. It looks like 100000 entries = ~10 seconds. The memory use when writing out the profile is now constant.

I used unconventional indentation in some places to better match beginning and ending of objects and arrays. I don't mind changing that, but maybe it's better to use RAII and keep the indents wherever possible? I think optional RAII would make the API a lot nicer, but I wanted to get some feedback before implementing it.

Comment 12

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json_gtest

I wrote some tests. I'm assuming that bug 986160 will be done before this one. This patch is missing the addition of the test to moz.build. I'll add that when bug 986160 is done and we are ready to commit this.

Comment 13

[Viktor Stanchev [:vikstrous]]

(In reply to J. Ryan Stinnett [:jryans] from comment #10)
> With bulk data, we'd be able to take any random nsIInputStream and stream it
> across the Dev Tools protocol's socket, as opposed to serializing tons of
> JSON as we do now.  So, if this change would make it possible to put the
> data into such a stream, that sounds great!
> 
> I don't know much about profiler internals...  It's not clear to me whether
> this change would bring us closer to or further from an input stream we
> could use with the Dev Tools.

Yeah, it should be easier to do that with this patch. I'm using std::ostream and just writing to it. I'm not familiar with nsIInputStream. Can we create a stream and pass one end to the profiler and one end to the socket and then have the profiler do blocking writes to the stream?

Comment 14

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json

There was a warning in the last patch that I forgot to fix. Here's the new try push: https://tbpl.mozilla.org/?tree=Try&rev=732525058bf9

Comment 15

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json

In this patch I removed the name filed from samples, so I needed to fix the tests that checks for that.

https://tbpl.mozilla.org/?tree=Try&rev=483b188c46be

Comment 16

[Viktor Stanchev [:vikstrous]]

I think the build on OSX is failing because of ostream or sstream. What should I use instead?

Comment 17

[Benoit Girard (:BenWa)]

(In reply to J. Ryan Stinnett [:jryans] from comment #10)

I'd imagine it would only be a small step to use an nsIInputStream from here.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Benoit Girard (:BenWa) from comment #17)
> (In reply to J. Ryan Stinnett [:jryans] from comment #10)
> 
> I'd imagine it would only be a small step to use an nsIInputStream from here.

Yay, that's what I was hoping would be the case.  It seemed that way to me as well.

Comment 19

[Benoit Girard (:BenWa)]

(In reply to Viktor Stanchev [:vikstrous] from comment #16)
> I think the build on OSX is failing because of ostream or sstream. What
> should I use instead?

We should be using that but we want to avoid polluting headers to prevent this problem. Maybe iosfwd.h can help here?

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8404822 [details] [diff] [review]
profiler_stream_json_gtest

Nice!

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8404928 [details] [diff] [review]
profiler_stream_json

Review of attachment 8404928 [details] [diff] [review]:
-----------------------------------------------------------------

Great work! It is overall much simpler minus some part of the profile writing code that got a bit more complex. All-in-all a nice win!

::: tools/profiler/GeckoProfiler.h
@@ +151,5 @@
>  // Get the profile encoded as a JSON object.
>  static inline JSObject* profiler_get_profile_jsobject(JSContext* aCx) { return nullptr; }
>  
> +// Get the profile written into a stream
> +static inline void profiler_stream_profile(std::ostream& aStream) { }

The include for ostream needs to go in this under outside the ifdef block otherwise ports with the profiler disabled will break.

::: tools/profiler/JSStreamWriter.cpp
@@ +15,5 @@
> + #define snprintf _snprintf
> +#endif
> +
> +static void* ARRAY = (void*)1;
> +static void* OBJECT = (void*)2;

can't we use #defines for this? Otherwise these should be const int instead.

@@ +61,5 @@
> +  }
> +  stream << "\"";
> +}
> +
> +JSStreamWriter::JSStreamWriter(std::ostream& aStream) : mStream(aStream), mNeedsComma(false), mNeedsName(false)

Style should be:
JSStreamWriter::JSStreamWriter(std::ostream& aStream)\n
__: mStream...
__, mNeedsC...

::: tools/profiler/JSStreamWriter.h
@@ +7,5 @@
> +#define JSSTREAMWRITER_H
> +
> +#include <ostream>
> +#include <stdlib.h>
> +#include "js/RootingAPI.h"

This header isn't needed.

@@ +13,5 @@
> +
> +class JSStreamWriter
> +{
> +public:
> +  // We need to ensure that this object lives on the stack so that GC sees it properly

This comment isn't needed.

::: tools/profiler/ProfileEntry.cpp
@@ +374,5 @@
> +
> +              sample = true;
> +
> +              // Seek forward through the entire sample, looking for frames
> +              // this is an easier to reason about approach than adding more

...an easier approach* to reason about than...

@@ +458,5 @@
> +  JSStreamWriter b(ss);
> +  StreamJSObject(b);
> +  NS_ConvertUTF8toUTF16 js_string(nsDependentCString(ss.str().c_str()));
> +  JS_ParseJSON(aCx, static_cast<const jschar*>(js_string.get()), js_string.Length(), &val);
> +  return &val.toObject();

Can rooted JS object be returned as a pointer? We should check that before landing.

Comment 22

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json2

https://tbpl.mozilla.org/?tree=Try&rev=2a9361834e9a

This seems to fix the OSX issue. I also fixed made most of the suggested changes.

I used non-const static void* because that's the API for nsDeque. I think defines might be the only way to guarantee that ARRAY and OBJECT are unchanged (aside from casting ARRAY and OBJECT every time they are used).

Comment 23

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json3

(In reply to Benoit Girard (:BenWa) from comment #21)
> Comment on attachment 8404928 [details] [diff] [review]
> profiler_stream_json
> 
> Review of attachment 8404928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +458,5 @@
> > +  JSStreamWriter b(ss);
> > +  StreamJSObject(b);
> > +  NS_ConvertUTF8toUTF16 js_string(nsDependentCString(ss.str().c_str()));
> > +  JS_ParseJSON(aCx, static_cast<const jschar*>(js_string.get()), js_string.Length(), &val);
> > +  return &val.toObject();
> 
> Can rooted JS object be returned as a pointer? We should check that before
> landing.

I made some changes based on my understanding of https://wiki.mozilla.org/Sfink/Draft_-_GC_Pointer_Handling

https://tbpl.mozilla.org/?tree=Try&rev=3c65e5d955e8

Comment 24

[Benoit Girard (:BenWa)]

Comment on attachment 8406398 [details] [diff] [review]
profiler_stream_json3

Review of attachment 8406398 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks big but it's actually a big simplification and rather poor patch diffing output. I just want to make sure we got JS rooting right. Please focus on tools/profiler/TableTicker.cpp line 169 where we return a pointer to a heap allocated rooted object.

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 8406398 [details] [diff] [review]
profiler_stream_json3

Review of attachment 8406398 [details] [diff] [review]:
-----------------------------------------------------------------

Please read and follow the guidelines in the JS GC rooting guide:
  https://developer.mozilla.org/en-US/docs/SpiderMonkey/GC_Rooting_Guide

::: tools/profiler/ProfileEntry.cpp
@@ +453,5 @@
>  
> +// The caller must delete the rooted JS value
> +JS::Rooted<JS::Value>* ThreadProfile::ToJSObject(JSContext *aCx)
> +{
> +  JS::Rooted<JS::Value>* val = new JS::Rooted<JS::Value>(aCx);

Ditto.

::: tools/profiler/TableTicker.cpp
@@ +170,2 @@
>  {
> +  JS::Rooted<JS::Value> *val = new JS::Rooted<JS::Value>(aCx);

This isn't the right usage; never use |new| with |Rooted|. Just make a Rooted on the stack here and return a normal JS::Value. The caller of ToJSObject is responsible for re-rooting the value if it needs to.

::: tools/profiler/nsProfiler.cpp
@@ +204,4 @@
>  NS_IMETHODIMP nsProfiler::GetProfileData(JSContext* aCx,
>                                           JS::MutableHandle<JS::Value> aResult)
>  {
> +  JS::Rooted<JS::Value> *obj = profiler_get_profile_jsobject(aCx);

Re-root rather than passing around pointers:
JS::Rooted<JS::Value> obj(aCx, ...);

::: tools/profiler/platform.cpp
@@ +550,4 @@
>    return profile;
>  }
>  
> +JS::Rooted<JS::Value>* mozilla_sampler_get_profile_data(JSContext *aCx)

Return a normal JS::Value here.

Comment 26

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json4

(In reply to Terrence Cole [:terrence] from comment #25)
> Comment on attachment 8406398 [details] [diff] [review]
> profiler_stream_json3
> 
> Review of attachment 8406398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please read and follow the guidelines in the JS GC rooting guide:
>   https://developer.mozilla.org/en-US/docs/SpiderMonkey/GC_Rooting_Guide

Thanks! I hadn't seen that. Reading it clarified things for me. The only change we would be making to the rooting stuff is in tools/profiler/nsProfiler.cpp where we were using a JS::Object pointer directly.

https://tbpl.mozilla.org/?tree=Try&rev=7b1cce48a8f5

Comment 27

[Terrence Cole [:terrence]]

Comment on attachment 8406961 [details] [diff] [review]
profiler_stream_json4

Review of attachment 8406961 [details] [diff] [review]:
-----------------------------------------------------------------

r=me Looks much more correct now, thanks!

::: tools/profiler/nsProfiler.cpp
@@ +204,4 @@
>  NS_IMETHODIMP nsProfiler::GetProfileData(JSContext* aCx,
>                                           JS::MutableHandle<JS::Value> aResult)
>  {
> +  JS::RootedObject obj(aCx, profiler_get_profile_jsobject(aCx));

Why did you remove the null check? I beleive you still need it. RootedObject has ConvertableToBool, so you can use it normally in bool contexts, so you can just keep the |if (!obj) return NS_ERROR_FAILURE;|.

Comment 28

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json5

Returned the null check.

Comment 29

[Viktor Stanchev [:vikstrous]]

I've tested desktop, b2g and android. I think it's safe to commit.

Comment 30

[Benoit Girard (:BenWa)]

Do you have a rebased version of these patches? They give out a large reject on inbound :(.

Comment 31

[Viktor Stanchev [:vikstrous]]

Attachment: profiler_stream_json6

Rebased.

https://tbpl.mozilla.org/?tree=Try&rev=18e71111e8c9

Comment 32

[Benoit Girard (:BenWa)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7aeb61045689
http://hg.mozilla.org/integration/mozilla-inbound/rev/19c2a56d49c2

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19c2a56d49c2
https://hg.mozilla.org/mozilla-central/rev/7aeb61045689