The default bug view has changed. See this FAQ.

Use JSON instead of string profiles on mobile

RESOLVED FIXED in mozilla16

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

unspecified
mozilla16
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #640881 - Flags: review?(sphink)
Attachment #640881 - Flags: review?(ehsan)
(Assignee)

Comment 1

5 years ago
Once this lands and all goes well I'll submit a patch to remove the text profile code entirely.
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.  :-)
Attachment #640881 - Flags: review?(ehsan) → review+
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?
Attachment #640881 - Flags: review?(sphink) → review+
(Assignee)

Comment 4

5 years ago
(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).
(Assignee)

Comment 5

5 years ago
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.
Attachment #640881 - Attachment is obsolete: true
Attachment #641168 - Flags: review+
(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.
(Assignee)

Comment 7

5 years ago
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.
Assignee: nobody → bgirard
Attachment #641168 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #641172 - Flags: review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5975dc46a539
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/5975dc46a539
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.