507448 - need a mechanism to retrieve function argument names for debugging

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Rob Campbell [:rc] (:robcee)]

filing this in JavaScript debugging for now, but may need to move to JS Engine.

Currently in Firebug (not sure how Venkman does it), we're calling function.toString() and parsing the returned value to extract the function names. We believe this is causing a large performance impact in some cases. For example, on pages using jQuery where the entire jQuery library is one large function.

Function.arguments.names was apparently removed in JS1.3 according to MDC.

https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Functions/arguments

Comment 1

[James Ross]

I assume "parsing the returned value to extract the function names" was meant to have "argument" before "names", right?

I am particularly curious at what point or under what pretense you actually need the function's formal parameter names, though; I don't believe Venkman ever gets them explictly (the closest is listing scope variables, where JSD provides the identification via flags).

Comment 2

[Boris Zbarsky [:bzbarsky]]

That's a good question.  Why does firebug need the argument names, exactly?

That said, there are some js internal APIs that have this information, more or less.  We'd just need to expose that to jsd, if we need it here.

robcee, do you want to take this?  And do you have a test page that shows the problem?

Comment 3

[John J. Barton]

I'm confused about the context here. A test case (page plus steps to reproduce) would help a lot. Where in Firebug do we use function.toString()?

Comment 4

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #1)
> I assume "parsing the returned value to extract the function names" was meant
> to have "argument" before "names", right?

yes, right.

> I am particularly curious at what point or under what pretense you actually
> need the function's formal parameter names, though; I don't believe Venkman
> ever gets them explictly (the closest is listing scope variables, where JSD
> provides the identification via flags).

We appear to be using the argument names as part of an internal representation for functions. Helps us when displaying callstacks and for watches.

(In reply to comment #3)
> I'm confused about the context here. A test case (page plus steps to reproduce)
> would help a lot. Where in Firebug do we use function.toString()?

in lib.js::safeToString() from getFunctionArgNames(). Boris was seeing a bunch of these during some profiling. Ultimately ended up as a toString() send to the function object.

(In reply to comment #2)
> That's a good question.  Why does firebug need the argument names, exactly?

as mentioned above, when we've got a JS Stack Frame, we go back through the list and reconstruct the arguments and value pairs, and possibly create these for watches as well.

Might be worth doing a deeper examination to make sure we're not using this excessively.

> That said, there are some js internal APIs that have this information, more or
> less.  We'd just need to expose that to jsd, if we need it here.
> 
> robcee, do you want to take this?  And do you have a test page that shows the
> problem?

Yeah, I'll take this. I don't have a test page yet, but it should be easy to create one using a bit of jQuery. I think you mentioned graphs.mozilla.org was a good culprit, but it's slow at the best of times anyway. Should be some good fodder in there for creating a test-case.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Just to be clear, I did NOT see this during profiling.  I saw it when I was trying to debug a bug that only showed up with Firebug installed and had my browser freezing up for tens of seconds at a time during the pageload of the site in question.

Then I profiled and set some breakpoints in the functions I saw being called.

I _think_ the bug I was looking at was bug 482293 and the site was http://www.panoramio.com/.  This has a jquery script which is basically a single 53-KB function.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I should note that at the time I did describe the problem to robcee, including the exact url where I was seeing it, etc.  :(

Comment 7

[Rob Campbell [:rc] (:robcee)]

ok, there was profiling done as a result of bug hunting. There was some profiling being done, which is all I was trying to say. I can check my logs, but the chances of finding that exact discussion is pretty slim. I wish I'd filed it at the time, but will try to get a testcase together with part of graphs or panoramio.

Comment 8

[Boris Zbarsky [:bzbarsky]]

My point was that this wasn't a case of "I was doing some profiling and noticed this taking up a few % of time" but a case of "This was making my browser completely unusable, and profiling was just how we figured out which exact code was making it unusable".

As in, this is one of the blockers for me using firebug on a regular basis.

Comment 9

[John J. Barton]

(In reply to comment #8) 
> As in, this is one of the blockers for me using firebug on a regular basis.

I would really like to see a Firebug bug report with any case that prevents you from being successful.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Will do; most of the issues I've run into so far have Mozilla bugs filed on them (since they need to be fixed on the jsd side, as far as I can tell).

I'll also send you some mail with more information; not sure it's relevant for bug reports.

Comment 11

[John J. Barton]

This bug came up in a discussion with Boris on another issue. 

Extending jsdIScript to add function argument names would be great. It should not be very difficult and it would make Firebug code more correct. (It would not make it faster because we will still need to parse for the function name which cannot be provided by jsdIScript).

However, if Firebug is calling toString() to get the argument names currently, that is a bug in Firebug. The only case where we have to decompile to get the source is browser-generated event handlers as far as I know.  In all other cases we have the source, jsdIScript points to that source, and we should be and have in the past extracted the arguments from that source.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> The only case where we have to decompile to get the source is browser-generated
> event handlers as far as I know.

Places you currently seem to decompile, at first glance:

findScriptForFunctionInContext in lib.js (called from various places in debugger.js and reps.js, as well as from findSourceForFunction in lib.js; this last is called from showFunction() in debugger.js and from inspectObject() in reps.js).

safeToString in lib.js, called from some reps.js stuff, but more importantly called from getFunctionArgNames in lib.js, which is called from getFunctionArgValues in lib.js.  Note that the callers of getFunctionArgValues are using script.functionObject as the object to pass to getFunctionArgValues.

Comment 13

[John J. Barton]

(In reply to comment #12)
> > The only case where we have to decompile to get the source is browser-generated
> > event handlers as far as I know.
> 
> Places you currently seem to decompile, at first glance:
> 
> findScriptForFunctionInContext in lib.js (called from various places in
> debugger.js and reps.js, as well as from findSourceForFunction in lib.js; this
> last is called from showFunction() in debugger.js and from inspectObject() in
> reps.js).

This code is a part of a mostly unsuccessful effort to improve the function support.

> 
> safeToString in lib.js, called from some reps.js stuff, but more importantly
> called from getFunctionArgNames in lib.js, which is called from
> getFunctionArgValues in lib.js.  Note that the callers of getFunctionArgValues
> are using script.functionObject as the object to pass to getFunctionArgValues.

Yes, and that code is bug, but not one we have any reports related to.

I can fix both of these.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Note that the getFunctionArgNames codepath is the one I ran into perf issues with.

Comment 15

[John J. Barton]

Ok here we are focusing on adding function argument support to jsdIScript.

For API we basically want an array of string. Here is how Firebug getFunctionArgNames:

    var values = [];

    var argNames = this.getFunctionArgNames(fn);
    for (var i = 0; i < argNames.length; ++i)
    {
        var argName = argNames[i];
        var pvalue = frame.scope.getProperty(argName);
        var value = pvalue ? pvalue.value.getWrappedValue() : undefined;
        values.push({name: argName, value: value});
    }

    return values;

For the actual API here are options: 

1) Follow the style of jsdIDebuggerSerivice, and add a method to jsdIScript:
    void enumerateArguments(in jsdIArgumentEnumerator enumerator);
  + fits in
  - old fashioned, 
  - more stuff than it needed for this simple feature.
I vote No.

2) Simple enumerator
    nsISimpleEnumerator enumerateArguments();
    + almost fits in
    + modern
    - no length, so we have to count to get number of args. But JS cares little about counts.

I vote yes.

3) jsiIValue
     jsdIValue getArguments();
      + fits in 
      - It implies backing by an object that does not exist? I guess the |arguments| thing in js is not available here?
      - requires a decoding book to explain how to use this 
          var arguments = script.getArguments();
          var listValue = {value: null}, lengthValue = {value: 0};
          arguments.getProperties(listValue, lengthValue);

Vote depends on questions.

4) nsIArray
     nsIArray arguments();
     + modern
     + length
     - more than we need for array of string     
This would just work for Firebug.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It implies backing by an object that does not exist? I guess the
> |arguments| thing in js is not available here?

Right.  It's not.

There's another option, I think:

5)  [array]

   getArguments(out unsigned long count,
                [array, size_is(count), retval] out AString arguments);

JS usage would look like:

   var arguments = script.getArguments({});

with |arguments| now being a JS Array containing some strings.  The need for the {} is annoying, and if this API is used from JS only maybe we should consider using nsIVariant instead if it ends up looking like a JS Array to the caller.  The upshot would be that you would just have a JS Array.

Would you prefer that to the enumerator?  Both are pretty easy to do....

Comment 17

[John J. Barton]

6) nsIEventListenerService.idl has this:

77   void getListenerInfoFor(in nsIDOMEventTarget aEventTarget,
78                           out unsigned long aCount,
79                           [retval, array, size_is(aCount)] out
80                             nsIEventListenerInfo aOutArray);

and the JS use is:
 var infos = els.getListenerInfoFor(elt);  // infos is an array
 That gives the array API without the funky object argument.

If that is not practical, I'd go for nsISimpleEnumerator.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Oh, it doesn't actually require the aCount arg from js?  If so, that's great.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I just checked, and the aCount arg is required.  Fix for that is in bug 523817.  ;)

So the API for this can just be:

  var arguments = script.getArguments();

with |arguments| being a JS Array of strings.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Josh, can you review the jsd changes?  Brendan, can you stamp the JS_FRIEND_API hole-poking?  And if you have time, also look over the jsd changes, just for sanity's sake?  My copy/paste fu is ok, but worth looking over.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 407784 [details] [diff] [review]
Fix

>+jsdScript::GetArgumentNames(PRUint32* count, PRUnichar*** arguments)
>+{
>+    ASSERT_VALID_EPHEMERAL;
>+    JSContext *cx = JSD_GetDefaultJSContext (mCx);
>+    if (!cx) {
>+        NS_WARNING("No default context !?");
>+        return NS_ERROR_FAILURE;
>+    }
>+    JSFunction *fun = JSD_GetJSFunction (mCx, mScript);
>+
>+    JSAutoRequest ar(cx);
>+
>+    if (fun && fun->hasLocalNames() && fun->nargs > 0) {
>+        PRUnichar **ret =
>+            static_cast<PRUnichar**>(NS_Alloc(fun->nargs * sizeof(PRUnichar*)));

Check for OOM.

>+
>+        void *mark = JS_ARENA_MARK(&cx->tempPool);
>+        jsuword *names = js_GetLocalNameArray(cx, fun, &cx->tempPool);
>+        if (!names) {
>+            NS_Free(ret);
>+            return NS_ERROR_OUT_OF_MEMORY;

You do here ;-).

>+        }
>+
>+        nsresult rv = NS_OK;
>+        for (uintN i = 0 ; i < fun->nargs; ++i) {
>+            JSAtom *atom = JS_LOCAL_NAME_TO_ATOM(names[i]);
>+            if (!atom) {
>+                ret[i] = 0;
>+            } else {
>+                jsval atomVal = ATOM_KEY(atom);
>+                if (!JSVAL_IS_STRING(atomVal)) {
>+                    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(i, ret);
>+                    rv = NS_ERROR_UNEXPECTED;
>+                    break;
>+                }
>+                JSString *str = JSVAL_TO_STRING(atomVal);
>+                ret[i] = NS_strndup(reinterpret_cast<PRUnichar*>(JS_GetStringChars(str)),
>+                                    JS_GetStringLength(str));
>+                if (!ret[i]) {
>+                    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(i, ret);
>+                    rv = NS_ERROR_OUT_OF_MEMORY;
>+                    break;
>+                }
>+            }
>+        }
>+        JS_ARENA_RELEASE(&cx->tempPool, mark);
>+        NS_ENSURE_SUCCESS(rv, rv);

I hate NS_ENSURE* but YMMV. Does jsd use it?

I guess jsd is already intimate with cx->tempPool :-(.

/be

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Check for OOM.

Good catch.

> I hate NS_ENSURE* but YMMV. Does jsd use it?

http://mxr.mozilla.org/mozilla-central/search?string=NS_ENSURE&find=js%2Fjsd&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Not much.  I can change to explicit check-and-return.

> I guess jsd is already intimate with cx->tempPool :-(.

It is not, in fact, but I was trying for minimal changes to js/src.  Do you prefer a larger js/src change so we can avoid tempPool here?

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> > I hate NS_ENSURE* but YMMV. Does jsd use it?
> 
> http://mxr.mozilla.org/mozilla-central/search?string=NS_ENSURE&find=js%2Fjsd&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
> 
> Not much.  I can change to explicit check-and-return.

That would be better IMHO.

> > I guess jsd is already intimate with cx->tempPool :-(.
> 
> It is not, in fact, but I was trying for minimal changes to js/src.  Do you
> prefer a larger js/src change so we can avoid tempPool here?

Not yet. :-)

/be

Comment 24

[timeless]

function annoying(a, b, a, b, b, a) {
}

i hope whatever your code does deals with this.

Comment 25

[timeless]

Comment on attachment 407784 [details] [diff] [review]
Fix

fwiw, the ns_ensure_true that exists today is something i checked in, dates to 2005, and was added w/ review from rginda and sr from brendan (bug 283532 comment 7), so yeah, we're fine w/ them :)

[note that some of the other ns_ensures are bogus and if i start getting people to review my patches, i'll post one to remove them]

That said, I think brendan's right that it doesn't really fit here.

>diff --git a/js/jsd/idl/jsdIDebuggerService.idl b/js/jsd>@@ -919,16 +919,22 @@ interface jsdIScript : jsdIEphemeral
>+     * The names of the arguments for this function; empty if this is
>+     * not a function.

would "The names of the declared arguments ..." be ok?

>diff --git a/js/jsd/jsd_xpc.cpp b/js/jsd/jsd_xpc.cpp

>+jsdScript::GetArgumentNames(PRUint32* count, PRUnichar*** arguments)
>+{

i'd prefer an early return using the else case here:
>+    if (fun && fun->hasLocalNames() && fun->nargs > 0) {

>+        PRUnichar **ret =
>+            static_cast<PRUnichar**>(NS_Alloc(fun->nargs * sizeof(PRUnichar*)));

i see brendan already flagged this :)

>+        nsresult rv = NS_OK;
>+        for (uintN i = 0 ; i < fun->nargs; ++i) {

i think the space before ; here doesn't belong.

>+            } else {
>+                if (!JSVAL_IS_STRING(atomVal)) {
...
>+                }

>+                if (!ret[i]) {
...
>+                }

i presume this loop isn't hot enough or interesting enough to justify collecting the code for those two cases together?

>+            }
>+        }

>diff --git a/js/jsd/test/Makefile.in b/js/jsd/test/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/js/jsd/test/Makefile.in

>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2007

I find this questionable in 2009, short of an actual hg copy command to show that you copied this form some existing file.

>new file mode 100644

I'd kinda request that there be a testcase with my annoying arguments or similar.

Comment 26

[Mike Shaver (:shaver emeritus)]

(In reply to comment #25)
> >diff --git a/js/jsd/idl/jsdIDebuggerService.idl b/js/jsd>@@ -919,16 +919,22 @@ interface jsdIScript : jsdIEphemeral
> >+     * The names of the arguments for this function; empty if this is
> >+     * not a function.
> 
> would "The names of the declared arguments ..." be ok?

Given

function f(a, b, c) { }
f(1, 2, 3);

I would expect "a", "b", and "c" to be called parameters, while "1", "2", and "3" are arguments.  I didn't think that arguments had names, really.

Am I alone in this?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> short of an actual hg copy command to show that you copied this form some
> existing file.

I did, in fact, but using cp, not hg copy...

> I'd kinda request that there be a testcase with my annoying arguments

Done.  Also added a testcase for what happens when we go over MAX_ARRAY_LOCALS (nothing special; it works fine).

Fixed the other review comments.

Shaver's right in comment 26; do we want this to be called getParameters or getParameterNames?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Oh, and:

> i presume this loop isn't hot enough or interesting enough to justify
> collecting the code for those two cases together?

Right.

Comment 29

[Boris Zbarsky [:bzbarsky]]

To be clear, if you prefer I can change the changeset around so the hg copy and ensuing edit is obvious in the changeset.  Or I can change the date, since this is basic boilerplate.  Whichever.  Just let me know.

Comment 30

[Brendan Eich [:brendan]]

getParameterNames.

"formal parameter" : "actual parameter" :: "parameter" : "argument"

Old school on left, often shortened to "formal" vs. "actual". New on right, the "a" for "actual" and "argument" helps.

/be

Comment 31

[John J. Barton]

(In reply to comment #26)
> 
> Given
> 
> function f(a, b, c) { }
> f(1, 2, 3);
> 
> I would expect "a", "b", and "c" to be called parameters, while "1", "2", and
> "3" are arguments.  I didn't think that arguments had names, really.
> 

What name do developers use for the value "3"? I guess the name "a". So we have argument values and argument names.  The "correct" alternatives are not as usable. But the API should be correct.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: With getParameterNames, hg cp

Comment 33

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/f501909820dc

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 408372 [details] [diff] [review]
With getParameterNames, hg cp

Would be good to get this on branch for Firebug.

Comment 35

[Rob Campbell [:rc] (:robcee)]

given that this was initially filed to help gain some performance improvements, I think we should try to get it into 1.9.2.

can I get a block here?

Comment 36

[Boris Zbarsky [:bzbarsky]]

This patch depends on bug 521010.  Can't land it on branch till that lands....

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 408372 [details] [diff] [review]
With getParameterNames, hg cp

a192=beltzner

Comment 38

[Rob Campbell [:rc] (:robcee)]

Boris, while trying to apply the patch (in attachment 408372 [details] [diff] [review]) to my local tree, I had a reject in js/jsd/test/Makefile.in. The whole thing's being replaced with one test entry for this bug. It's probably right, but I don't want to wipe out any tests that this might be running. Could you let me know if that makefile is as you intended it?

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Could you let me know if that makefile is as you intended it?

Yes; the text being replaced is content/base/test/Makefile.in.  See comment 29.

How about I just attach the 1.9.2 merge of this I have sitting in my mq, so you can just use that?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.2 merge

Comment 41

[Rob Campbell [:rc] (:robcee)]

even better. Thanks!

Comment 42

[Rob Campbell [:rc] (:robcee)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3f226e4fc951

Comment 43

[Boris Zbarsky [:bzbarsky]]

Please set the status1.9.2 flag when pushing to branch?