Last Comment Bug 772715 - Use JSON instead of string profiles on mobile
: Use JSON instead of string profiles on mobile
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-10 18:10 PDT by Benoit Girard (:BenWa)
Modified: 2012-07-12 09:38 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use JSON profiles on mobile (4.42 KB, patch)
2012-07-10 18:10 PDT, Benoit Girard (:BenWa)
sphink: review+
ehsan: review+
Details | Diff | Splinter Review
patch (7.78 KB, patch)
2012-07-11 12:48 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
patch (fix rot) (5.02 KB, patch)
2012-07-11 12:59 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-07-10 18:10:32 PDT
Created attachment 640881 [details] [diff] [review]
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) 2012-07-10 18:11:10 PDT
Once this lands and all goes well I'll submit a patch to remove the text profile code entirely.
Comment 2 :Ehsan Akhgari 2012-07-10 19:09:56 PDT
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:] (PTO Sep23-28) 2012-07-11 11:38:43 PDT
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) 2012-07-11 12:13:15 PDT
(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) 2012-07-11 12:48:36 PDT
Created attachment 641168 [details] [diff] [review]
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:] (PTO Sep23-28) 2012-07-11 12:56:36 PDT
(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) 2012-07-11 12:59:19 PDT
Created attachment 641172 [details] [diff] [review]
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 9 Ed Morley [:emorley] 2012-07-12 09:38:31 PDT
https://hg.mozilla.org/mozilla-central/rev/5975dc46a539

Note You need to log in before you can comment on or make changes to this bug.