Closed
Bug 518055
Opened 15 years ago
Closed 15 years ago
Turn recursive descent parsing functions into JSCompiler methods; remove parameters
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jimb, Assigned: dherman)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 4 obsolete files)
182.64 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•15 years ago
|
||
I hereby steal all the therapy for myself, due to my gumption.
Assignee: general → cleary
Assignee | ||
Comment 2•15 years ago
|
||
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
Assignee | ||
Comment 3•15 years ago
|
||
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
Assignee: cdleary → dherman
Assignee | ||
Comment 4•15 years ago
|
||
***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.
Assignee | ||
Comment 5•15 years ago
|
||
- 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
Attachment #430884 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #431143 -
Flags: review?(jim)
Assignee | ||
Comment 6•15 years ago
|
||
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
Comment 7•15 years ago
|
||
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
Assignee | ||
Comment 8•15 years ago
|
||
> 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
Comment 9•15 years ago
|
||
(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
Assignee | ||
Comment 10•15 years ago
|
||
***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
Attachment #431143 -
Attachment is obsolete: true
Attachment #431383 -
Flags: review?(jim)
Attachment #431143 -
Flags: review?(jim)
Comment 11•15 years ago
|
||
Nit: lowerThenCamel is called camelCaps, hump is in the middle ;-).
/be
Assignee | ||
Comment 12•15 years ago
|
||
> Nit: lowerThenCamel is called camelCaps, hump is in the middle ;-).
Right you are. Too many years with some-other-language, I guess. ;)
Dave
Reporter | ||
Comment 13•15 years ago
|
||
Comment on attachment 431383 [details] [diff] [review]
class-ification of parser as JSCompiler methods
Needs revision for resurrection of bug 551021.
Attachment #431383 -
Flags: review?(jim) → review-
Reporter | ||
Comment 14•15 years ago
|
||
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.
Attachment #431383 -
Flags: review- → review?(jim)
Reporter | ||
Comment 15•15 years ago
|
||
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?
Reporter | ||
Comment 16•15 years ago
|
||
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.
Attachment #431383 -
Flags: review?(jim) → review+
Assignee | ||
Comment 17•15 years ago
|
||
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
Assignee | ||
Comment 18•15 years ago
|
||
Implemented jimb's recommendations, including manual s/Foo/foo/g through all comments in 10KLOC. Respect.
Dave
Attachment #431383 -
Attachment is obsolete: true
Assignee | ||
Comment 19•15 years ago
|
||
hg qrefresh -U
Prêt à importer, as it were.
Dave
Attachment #431973 -
Attachment is obsolete: true
Comment 20•15 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 21•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•