Closed
Bug 507448
Opened 15 years ago
Closed 15 years ago
need a mechanism to retrieve function argument names for debugging
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta2-fixed |
People
(Reporter: rcampbell, Assigned: bzbarsky)
References
Details
(Keywords: perf, Whiteboard: [firebug-p1])
Attachments
(2 files, 1 obsolete file)
17.33 KB,
patch
|
beltzner
:
approval1.9.2+
|
Details | Diff | Splinter Review |
17.28 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•15 years ago
|
OS: Mac OS X → All
Whiteboard: [firebug-p1]
Assignee: nobody → general
Component: JavaScript Debugging APIs → JavaScript Engine
QA Contact: jsd → general
Comment 1•15 years ago
|
||
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).
![]() |
Assignee | |
Comment 2•15 years ago
|
||
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•15 years ago
|
||
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()?
Reporter | ||
Comment 4•15 years ago
|
||
(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.
![]() |
Assignee | |
Comment 5•15 years ago
|
||
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.
![]() |
Assignee | |
Comment 6•15 years ago
|
||
I should note that at the time I did describe the problem to robcee, including the exact url where I was seeing it, etc. :(
Reporter | ||
Comment 7•15 years ago
|
||
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.
![]() |
Assignee | |
Comment 8•15 years ago
|
||
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•15 years ago
|
||
(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.
![]() |
Assignee | |
Comment 10•15 years ago
|
||
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•15 years ago
|
||
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.
![]() |
Assignee | |
Comment 12•15 years ago
|
||
> 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•15 years ago
|
||
(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.
![]() |
Assignee | |
Comment 14•15 years ago
|
||
Note that the getFunctionArgNames codepath is the one I ran into perf issues with.
Comment 15•15 years ago
|
||
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.
![]() |
Assignee | |
Comment 16•15 years ago
|
||
> 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•15 years ago
|
||
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.
![]() |
Assignee | |
Comment 18•15 years ago
|
||
Oh, it doesn't actually require the aCount arg from js? If so, that's great.
![]() |
Assignee | |
Comment 19•15 years ago
|
||
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.
Depends on: 523817
![]() |
Assignee | |
Comment 20•15 years ago
|
||
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.
![]() |
Assignee | |
Updated•15 years ago
|
Component: JavaScript Engine → JavaScript Debugging APIs
QA Contact: general → jsd
![]() |
Assignee | |
Updated•15 years ago
|
Attachment #407784 -
Flags: review?(timeless)
Comment 21•15 years ago
|
||
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
Attachment #407784 -
Flags: review?(brendan) → review+
![]() |
Assignee | |
Comment 22•15 years ago
|
||
> 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•15 years ago
|
||
(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•15 years ago
|
||
function annoying(a, b, a, b, b, a) { } i hope whatever your code does deals with this.
Comment 25•15 years ago
|
||
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.
Attachment #407784 -
Flags: review?(timeless) → review+
Comment 26•15 years ago
|
||
(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?
![]() |
Assignee | |
Comment 27•15 years ago
|
||
> 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?
![]() |
Assignee | |
Comment 28•15 years ago
|
||
Oh, and:
> i presume this loop isn't hot enough or interesting enough to justify
> collecting the code for those two cases together?
Right.
![]() |
Assignee | |
Comment 29•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
(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.
![]() |
Assignee | |
Comment 33•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/f501909820dc
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
![]() |
Assignee | |
Updated•15 years ago
|
Flags: blocking1.9.2?
![]() |
Assignee | |
Comment 34•15 years ago
|
||
Comment on attachment 408372 [details] [diff] [review] With getParameterNames, hg cp Would be good to get this on branch for Firebug.
Attachment #408372 -
Flags: approval1.9.2?
Reporter | ||
Comment 35•15 years ago
|
||
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?
Flags: wanted1.9.2?
Keywords: perf
Reporter | ||
Updated•15 years ago
|
Whiteboard: [firebug-p1] → [firebug-p1][branch-approval][branch-landing][firebug-blocks]
Updated•15 years ago
|
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
![]() |
Assignee | |
Comment 36•15 years ago
|
||
This patch depends on bug 521010. Can't land it on branch till that lands....
Depends on: 521010
Comment 37•15 years ago
|
||
Comment on attachment 408372 [details] [diff] [review] With getParameterNames, hg cp a192=beltzner
Attachment #408372 -
Flags: approval1.9.2? → approval1.9.2+
Reporter | ||
Comment 38•15 years ago
|
||
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?
![]() |
Assignee | |
Comment 39•15 years ago
|
||
> 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?
![]() |
Assignee | |
Comment 40•15 years ago
|
||
Reporter | ||
Comment 42•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3f226e4fc951
Whiteboard: [firebug-p1][branch-approval][branch-landing][firebug-blocks] → [firebug-p1]
![]() |
Assignee | |
Comment 43•15 years ago
|
||
Please set the status1.9.2 flag when pushing to branch?
status1.9.2:
--- → final-fixed
Updated•13 years ago
|
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
You need to log in
before you can comment on or make changes to this bug.
Description
•