Stream profiler JSON directly to a file

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: BenWa, Assigned: vikstrous)

Tracking

Trunk
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

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.
Duplicate of this bug: 937852
Assignee: nobody → me
Assignee: me → vstanchev
Duplicate of this bug: 989474
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.
Flags: needinfo?(bgirard)
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...
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.
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.
Flags: needinfo?(bgirard)
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?
Flags: needinfo?(bgirard)
Depends on: 992251
No longer depends on: 992251
(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.
Flags: needinfo?(bgirard)
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.
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.
Posted patch profiler_stream_json (obsolete) — Splinter Review
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.
Attachment #8404819 - Flags: review?(bgirard)
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.
Attachment #8404822 - Flags: review?(bgirard)
(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?
Posted patch profiler_stream_json (obsolete) — Splinter Review
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
Attachment #8404819 - Attachment is obsolete: true
Attachment #8404819 - Flags: review?(bgirard)
Attachment #8404855 - Flags: review?(bgirard)
Posted patch profiler_stream_json (obsolete) — Splinter Review
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
Attachment #8404855 - Attachment is obsolete: true
Attachment #8404855 - Flags: review?(bgirard)
Attachment #8404928 - Flags: review?(bgirard)
I think the build on OSX is failing because of ostream or sstream. What should I use instead?
Flags: needinfo?(bgirard)
(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.
(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.
(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?
Flags: needinfo?(bgirard)
Comment on attachment 8404822 [details] [diff] [review]
profiler_stream_json_gtest

Nice!
Attachment #8404822 - Flags: review?(bgirard) → review+
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.
Attachment #8404928 - Flags: review?(bgirard) → review+
Posted patch profiler_stream_json2 (obsolete) — Splinter Review
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).
Attachment #8404928 - Attachment is obsolete: true
Attachment #8406283 - Flags: review?(bgirard)
Attachment #8406283 - Flags: review?(bgirard)
Attachment #8406283 - Flags: review+
Posted patch profiler_stream_json3 (obsolete) — Splinter Review
(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
Attachment #8406283 - Attachment is obsolete: true
Attachment #8406398 - Flags: review?(bgirard)
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.
Attachment #8406398 - Flags: review?(terrence)
Attachment #8406398 - Flags: review?(bgirard)
Attachment #8406398 - Flags: review+
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.
Attachment #8406398 - Flags: review?(terrence) → review-
Posted patch profiler_stream_json4 (obsolete) — Splinter Review
(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
Attachment #8406398 - Attachment is obsolete: true
Attachment #8406961 - Flags: review?(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;|.
Attachment #8406961 - Flags: review?(terrence) → review+
Posted patch profiler_stream_json5 (obsolete) — Splinter Review
Returned the null check.
Attachment #8406961 - Attachment is obsolete: true
I've tested desktop, b2g and android. I think it's safe to commit.
Flags: needinfo?(bgirard)
Do you have a rebased version of these patches? They give out a large reject on inbound :(.
Flags: needinfo?(bgirard) → needinfo?(vstanchev)
Rebased.

https://tbpl.mozilla.org/?tree=Try&rev=18e71111e8c9
Attachment #8406986 - Attachment is obsolete: true
Flags: needinfo?(vstanchev)
https://hg.mozilla.org/mozilla-central/rev/19c2a56d49c2
https://hg.mozilla.org/mozilla-central/rev/7aeb61045689
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1005204
You need to log in before you can comment on or make changes to this bug.