Turn recursive descent parsing functions into JSCompiler methods; remove parameters

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jimb, Assigned: dherman)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Assignee: general → cleary
(Assignee)

Comment 2

9 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

9 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)

Updated

9 years ago
Depends on: 550629
(Assignee)

Updated

9 years ago
No longer depends on: 550629
(Assignee)

Updated

9 years ago
Blocks: 548461
(Assignee)

Comment 4

9 years ago
Created attachment 430884 [details] [diff] [review]
class-ification of parser as JSCompiler methods

***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

9 years ago
Created attachment 431143 [details] [diff] [review]
class-ification of parser as JSCompiler methods

- 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

9 years ago
Attachment #431143 - Flags: review?(jim)
(Assignee)

Comment 6

9 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
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

9 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
(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

9 years ago
Created attachment 431383 [details] [diff] [review]
class-ification of parser as JSCompiler methods

***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)
Nit: lowerThenCamel is called camelCaps, hump is in the middle ;-).

/be
(Assignee)

Comment 12

9 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

9 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

9 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

9 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

9 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

9 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

9 years ago
Created attachment 431973 [details] [diff] [review]
class-ification of parser as JSCompiler methods

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

9 years ago
Created attachment 431976 [details] [diff] [review]
class-ification of parser as JSCompiler methods

hg qrefresh -U

Prêt à importer, as it were.

Dave
Attachment #431973 - Attachment is obsolete: true

Comment 21

9 years ago
http://hg.mozilla.org/mozilla-central/rev/92ab997f7c8f
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.