Brendan suggested the cleanups below in bug 517990, and asked: > Jim: you want to do it? I was going to do it as parasympathetic nervous system > therapy some night, but it's fair game for anyone. Sure --- I've promised strict mode and breakpad CFI at the moment, but once those are in the clear, this would be satisfying to do. But it's filed as a bug now, so anyone with gumption should grab it. --- The bigger cleanup I kept refraining from doing in the upvar2 work, to keep the patch only insanely big instead of super-insanely big, is to get rid of almost all the parameters to jsparse.cpp functions, making most of them methods of JSCompiler. This should speed up the recursive-descent parser. Beyond this there are some cleanups to jsscan.h to use C++ canonical style. But again the JSCompiler wants at least some inline helpers to forward calls on itself to its tokenStream member, to save typing and shorten lines full of redundancy in jsparse.cpp. --- To say a bit more, all the C static functions that have the JSParser signature: typedef JSParseNode * JSParser(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc); become JSCompiler methods taking zero args. Any cx uses become free and need to rename to context -- or better, we rename JSCompiler::context -> cx. Similarly for ts -> &tokenStream, mutatis mutandis. The tc param needs to be a top of tree context stack pointer in JSCompiler, but that's easy to do too. Hope this helps, and actually wins. Zero params has to make a difference on code perf as well as size, even if we sometimes load this->cx or whatever. But a second opinion is welcome.

I hereby steal all the therapy for myself, due to my gumption.

I may have exhibited some pre-emptive gumption-- my work on bug #548461 overlaps with this one. So far it hasn't turned out to be a performance win, though, so I'm currently trying to split it up a bit, starting with bug #550350. Dave

Chris and I are sharing the therapy: he's split out the jsscan changes in bug 549658, and I'm doing the JSCompiler stuff here, so I'm reassigning this one to me. Dave

***NB: To apply this patch you need to apply the patch attached to bug 550350 first.*** This patch turns the recursive-descent parser functions into methods of JSCompiler. (For bug 548461 the methods should eventually become JSSourceCompiler methods instead.) It passes all tests and according to Leary's test suite in bug 548621 doesn't affect performance significantly.

- update patch to work off today's tracemonkey tree - minor style edit: shorten over-long js_ReportCompileErrorNumber lines - change JSCompiler::parseXXX member functions to private

I split off removal of JSTreeContext* parameter as separate bug 551021. Hope I'm not going too far with the sub-division of bugs. But this way we can test them separately for performance. Dave

I know we usually verbThingie when naming methods, but parsing methods were all named without the implied "parse", and besides scratching the itch that makes all hackers love to verb nouns (and noun verbs), this seems winning to reduce noise and redundancy. My two cents, /be

> I know we usually verbThingie when naming methods, but parsing methods were all > named without the implied "parse", and besides scratching the itch that makes > all hackers love to verb nouns (and noun verbs), this seems winning to reduce > noise and redundancy. My two cents, I wasn't really sure about it; I think I was tempted to add the prefix as a way to keep the CapitalizedProductionName to match the BNF (since it appears the convention for method names is lowercaseThenCamelCase, right?). But yeah, it adds entropy, and since they're relatively pure, naming the thing they produce ("statement") instead of their computational behavior ("parse a statement") seems appropriate. So... "statements" and "parenExpr" instead of "parseStatements" and "parseParenExpr"? Dave

(In reply to comment #8) > > I know we usually verbThingie when naming methods, but parsing methods were all > > named without the implied "parse", and besides scratching the itch that makes > > all hackers love to verb nouns (and noun verbs), this seems winning to reduce > > noise and redundancy. My two cents, > > I wasn't really sure about it; I think I was tempted to add the prefix as a way > to keep the CapitalizedProductionName to match the BNF (since it appears the > convention for method names is lowercaseThenCamelCase, right?) ECMA-262 non-terminal names are indeed rendered in StudlyCaps but they are different from our names (spelled-out Expression, e.g.). > So... "statements" and "parenExpr" instead of "parseStatements" and > "parseParenExpr"? Right. /be

***NB: As before, to apply this patch you need to apply the patch attached to bug 550350 first.*** Updated to use lowerThenCamel parser method names instead of parseStudlyCaps, per Brendan's suggestion. Dave

Nit: lowerThenCamel is called camelCaps, hump is in the middle ;-). /be

> Nit: lowerThenCamel is called camelCaps, hump is in the middle ;-). Right you are. Too many years with some-other-language, I guess. ;) Dave

Comment on attachment 431383 [details] [diff] [review] class-ification of parser as JSCompiler methods Needs revision for resurrection of bug 551021.

Comment on attachment 431383 [details] [diff] [review] class-ification of parser as JSCompiler methods Sorry, thought 551021 was a prereq for this one, not the reverse.

I'm about 15% through, but I need to step out for dinner/school meeting. I will finish this up when I get back tonight. Some minor thoughts: Would it be more consistent to use 'context' (the member) instead of 'cx' in JSCompiler::compileScript and the other entry points, for consistency with the non-static member functions of JSCompiler? >+ * later, because we may have not peeked in tokenStream yet, so Statements That's 'statements' now, isn't it?

Comment on attachment 431383 [details] [diff] [review] class-ification of parser as JSCompiler methods Looks good, with the nits noted below fixed. >@@ -975,17 +925,18 @@ JSCompiler::compileScript(JSContext *cx, > #if JS_HAS_XML_SUPPORT > /* > * Prevent XML data theft via <script src="http://victim.com/foo.xml">. > * For background, see: > * > * https://bugzilla.mozilla.org/show_bug.cgi?id=336551 > */ > if (pn && onlyXML && (tcflags & TCF_NO_SCRIPT_RVAL)) { >- js_ReportCompileErrorNumber(cx, &jsc.tokenStream, NULL, JSREPORT_ERROR, >+ js_ReportCompileErrorNumber(cx, &jsc.tokenStream, NULL, >+ JSREPORT_ERROR, > JSMSG_XML_WHOLE_PROGRAM); I think the current fashion would consider this change a waste of vertical space. The width limit in SpiderMonkey nowadays is 100 columns, so go ahead and leave the JSREPORT_ERROR on the same line. There are a number of places like this, all involving js_ReportCompileErrorNumber. As a separate patch, it might be nice to replace the calls to js_ReportCompileErrorNumber with calls to a JSCompiler member function that can supply the cx and ts arguments. >-static JSParseNode * >-XMLExpr(JSContext *cx, JSTokenStream *ts, JSBool inTag, JSTreeContext *tc) >+JSParseNode * >+JSCompiler::xmlExpr(JSBool inTag, JSTreeContext *tc) What?!? Not xMLExpr? :) >- * necessary for safe cx->display-based optimization of >- * the closure's static link. >+ * necessary for safe context->display-based optimiza- >+ * tion of the closure's static link. Again, the 100 column limit.

Many thanks, jimb. Updated patch forthcoming. > As a separate patch, it might be nice to replace the calls to > js_ReportCompileErrorNumber with calls to a JSCompiler member function that can > supply the cx and ts arguments. See bug 551072. Dave

Implemented jimb's recommendations, including manual s/Foo/foo/g through all comments in 10KLOC. Respect. Dave

hg qrefresh -U Prêt à importer, as it were. Dave

Pushed http://hg.mozilla.org/tracemonkey/rev/92ab997f7c8f