772715 - Use JSON instead of string profiles on mobile

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: Use JSON profiles on mobile

The text format has a lot of limitations and is buggy and I don't want to maintain it any longer. This should be the final pieces needed to remove JSON.

Comment 1

[Benoit Girard (:BenWa)]

Once this lands and all goes well I'll submit a patch to remove the text profile code entirely.

Comment 2

[(no longer active)]

Comment on attachment 640881 [details] [diff] [review]
Use JSON profiles on mobile

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

r=me (not on the js engine bits)

::: tools/profiler/TableTicker.cpp
@@ +427,5 @@
> +static JSBool
> +WriteCallback(const jschar *buf, uint32_t len, void *data)
> +{
> +  std::ofstream& stream = *static_cast<std::ofstream*>(data);
> +  nsCAutoString profile = NS_ConvertUTF16toUTF8(buf, len);

I'm not really sure if this code is valid for jschar.  Please check with someone who knows things about the js engine.  :-)

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 640881 [details] [diff] [review]
Use JSON profiles on mobile

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

::: tools/profiler/TableTicker.cpp
@@ +427,5 @@
> +static JSBool
> +WriteCallback(const jschar *buf, uint32_t len, void *data)
> +{
> +  std::ofstream& stream = *static_cast<std::ofstream*>(data);
> +  nsCAutoString profile = NS_ConvertUTF16toUTF8(buf, len);

I don't know either, but I notice that xpconnect uses it in a couple places where the string's value is fairly controlled, which I'd guess is true of here. You could look at XPCConvert::JSData2Native in js/xpconnect/src/XPCConvert.cpp for how it seems to be done generally. There are a bunch of different cases, though.

@@ +479,5 @@
> +      LOG("Failed to get context");
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    JS_BeginRequest(cx);

Not strictly necessary, but you might want to use JSAutoRequest here.

@@ +486,5 @@
> +          "global", JSCLASS_GLOBAL_FLAGS,
> +          JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_StrictPropertyStub,
> +          JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub
> +      };
> +      JSObject *obj = JS_NewGlobalObject(cx, &c, NULL);

Wow, this is completely crazy. Probably necessary, though, since you can't depend on any JS running so you can't use the context stack.

How often is this called? Is it infrequent, or should you keep a pet JSContext around all the time?

@@ +498,5 @@
> +      // being thread safe. Bug 750989.
> +      t->SetPaused(true);
> +      if (stream.is_open()) {
> +        JSAutoEnterCompartment autoComp;
> +        if (autoComp.enter(cx, obj)) {

Probably ought to fail and/or log if this fails.

@@ +504,5 @@
> +          jsval val = OBJECT_TO_JSVAL(profileObj);
> +          JS_Stringify(cx, &val, nsnull, JSVAL_NULL, WriteCallback, &stream);
> +        }
> +        stream.close();
> +        LOG("Saved to " FOLDER "profile_TYPE_PID.txt");

Are TYPE and PID todos?

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Steve Fink [:sfink] from comment #3)
> Comment on attachment 640881 [details] [diff] [review]
> Use JSON profiles on mobile
> 
> Review of attachment 640881 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/TableTicker.cpp
> @@ +427,5 @@
> > +static JSBool
> > +WriteCallback(const jschar *buf, uint32_t len, void *data)
> > +{
> > +  std::ofstream& stream = *static_cast<std::ofstream*>(data);
> > +  nsCAutoString profile = NS_ConvertUTF16toUTF8(buf, len);
> 
> I don't know either, but I notice that xpconnect uses it in a couple places
> where the string's value is fairly controlled, which I'd guess is true of
> here. You could look at XPCConvert::JSData2Native in
> js/xpconnect/src/XPCConvert.cpp for how it seems to be done generally. There
> are a bunch of different cases, though.
> 

I'll have a look but the input is fairly controlled so if it's not simple I'll keep this code for now until we see a problem if you don't mind.

> @@ +479,5 @@
> > +      LOG("Failed to get context");
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +
> > +    JS_BeginRequest(cx);
> 
> Not strictly necessary, but you might want to use JSAutoRequest here.
> 

Easy enough.

> @@ +486,5 @@
> > +          "global", JSCLASS_GLOBAL_FLAGS,
> > +          JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_StrictPropertyStub,
> > +          JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub
> > +      };
> > +      JSObject *obj = JS_NewGlobalObject(cx, &c, NULL);
> 
> Wow, this is completely crazy. Probably necessary, though, since you can't
> depend on any JS running so you can't use the context stack.
> 
> How often is this called? Is it infrequent, or should you keep a pet
> JSContext around all the time?
> 

This code is called when we save the profile on mobile to a file. It's a very rare and user initiated operation that is already several seconds long. I don't care about fast performance here.

> @@ +498,5 @@
> > +      // being thread safe. Bug 750989.
> > +      t->SetPaused(true);
> > +      if (stream.is_open()) {
> > +        JSAutoEnterCompartment autoComp;
> > +        if (autoComp.enter(cx, obj)) {
> 
> Probably ought to fail and/or log if this fails.
> 
> @@ +504,5 @@
> > +          jsval val = OBJECT_TO_JSVAL(profileObj);
> > +          JS_Stringify(cx, &val, nsnull, JSVAL_NULL, WriteCallback, &stream);
> > +        }
> > +        stream.close();
> > +        LOG("Saved to " FOLDER "profile_TYPE_PID.txt");
> 
> Are TYPE and PID todos?

Not a typo but glandium has a patch to improve the naming to use proper printf that should land. I'll handle that in a new patch. (we used to run this code in a signal printf wasn't an option).

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch

Carry forward r+.

I didn't use JSData2Native since you need an XPConnect context if that's ok with you.

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to Benoit Girard (:BenWa) from comment #5)
> Created attachment 641168 [details] [diff] [review]
> patch
> 
> I didn't use JSData2Native since you need an XPConnect context if that's ok
> with you.

Sorry, I didn't even mean to suggest that. I was just saying that you might want to look at it if you needed to convert fully general strings. I don't think you need it, though; afaict, what you're doing is fine.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch (fix rot)

This patch fixes the bit rot (glandium landed his change to properly name and log profiles). Also the previous had some extra diff merged from another bug, this version should be good.

Comment 8

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5975dc46a539

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5975dc46a539