Bug 71138 - Towards 8-bit Strings: Move Lexer and Parser Objects out of JSGlobalData
Summary: Towards 8-bit Strings: Move Lexer and Parser Objects out of JSGlobalData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-10-28 13:49 PDT by Michael Saboff
Modified: 2012-02-17 14:04 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch (441.23 KB, patch)
2011-10-28 14:10 PDT, Michael Saboff
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch with speculative build fixes (444.06 KB, patch)
2011-10-28 16:05 PDT, Michael Saboff
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-10-28 13:49:06 PDT
Currently JSGlobalData has Lexer and Parser objects that are used for JavaScript parsing.  These "singletons" are reset and reused for each parse task. As part of the overall 8-bit string changes, the Lexer (and therefore Parser) will be templatized on the character storage size and therefore need to be restructured.  Since parsing if a localized task (primarily in JavaScriptCore/runtime/Executable.cpp) it makes sense to create, use and destroy parser objects as needed.

<rdar://problem/10225268>
Comment 1 Michael Saboff 2011-10-28 14:10:29 PDT
Created attachment 112908 [details]
Proposed patch
Comment 2 Early Warning System Bot 2011-10-28 14:41:42 PDT
Comment on attachment 112908 [details]
Proposed patch

Attachment 112908 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10237347
Comment 3 Gustavo Noronha (kov) 2011-10-28 15:10:49 PDT
Comment on attachment 112908 [details]
Proposed patch

Attachment 112908 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10236399
Comment 4 Michael Saboff 2011-10-28 15:20:41 PDT
(In reply to comment #3)
> (From update of attachment 112908 [details])
> Attachment 112908 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/10236399

Yeah, I forgot to remove JSParser.{cpp,h} from all build files.  This looks like the source of the qt, gtk, win and likely efl build issues.

I'll provide an updated patch with speculative changes for these platforms.
Comment 5 Gyuyoung Kim 2011-10-28 15:28:05 PDT
Comment on attachment 112908 [details]
Proposed patch

Attachment 112908 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10236406
Comment 6 Michael Saboff 2011-10-28 16:05:21 PDT
Created attachment 112935 [details]
Updated patch with speculative build fixes
Comment 7 Darin Adler 2011-10-28 17:13:07 PDT
Comment on attachment 112935 [details]
Updated patch with speculative build fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=112935&action=review

The thing I don’t like here is that some day we might want the source code location information to be more precise than just a line number. If we do, the we have to go back again and change all these call sites to pass something other than a line number. Is there some way we can avoid being so explicit about the line number? Any reason not to use something in global data instead of passing the arguments? Or could we at least pass a struct that has a line number in it instead of just passing the int?

> Source/JavaScriptCore/parser/ASTBuilder.h:146
> +    ExpressionNode* makeAssignNode(int, ExpressionNode* left, Operator, ExpressionNode* right, bool leftHasAssignments, bool rightHasAssignments, int start, int divot, int end);
> +    ExpressionNode* makePrefixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> +    ExpressionNode* makePostfixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> +    ExpressionNode* makeTypeOfNode(int, ExpressionNode*);
> +    ExpressionNode* makeDeleteNode(int , ExpressionNode*, int start, int divot, int end);
> +    ExpressionNode* makeNegateNode(int, ExpressionNode*);
> +    ExpressionNode* makeBitwiseNotNode(int, ExpressionNode*);
> +    ExpressionNode* makeMultNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeDivNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeModNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeAddNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeSubNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeBitXOrNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeBitAndNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeBitOrNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeLeftShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeRightShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> +    ExpressionNode* makeURightShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);

These should all say lineNumber. It’s not at all obvious that the integer is a line number. Also, there is an extra space on the makeDeleteNode line.
Comment 8 Michael Saboff 2011-10-28 17:28:33 PDT
(In reply to comment #7)
> (From update of attachment 112935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112935&action=review
> 
> The thing I don’t like here is that some day we might want the source code location information to be more precise than just a line number. If we do, the we have to go back again and change all these call sites to pass something other than a line number. Is there some way we can avoid being so explicit about the line number? Any reason not to use something in global data instead of passing the arguments? Or could we at least pass a struct that has a line number in it instead of just passing the int?

I agree that it may make sense to have greater context, e.g. line number and position or column number, but I don't want to complicate the code for a possible future enhancement. I also think it would be safer not to use JSGlobalData as the source of that context.  This patch moves the source of the line number, the Lexer object, out of the JSGlobalData. Those two reasons suggest to me that we replace the lineNumber parameter with a context object at some future date.

> > Source/JavaScriptCore/parser/ASTBuilder.h:146
> > +    ExpressionNode* makeAssignNode(int, ExpressionNode* left, Operator, ExpressionNode* right, bool leftHasAssignments, bool rightHasAssignments, int start, int divot, int end);
> > +    ExpressionNode* makePrefixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> > +    ExpressionNode* makePostfixNode(int, ExpressionNode*, Operator, int start, int divot, int end);
> > +    ExpressionNode* makeTypeOfNode(int, ExpressionNode*);
> > +    ExpressionNode* makeDeleteNode(int , ExpressionNode*, int start, int divot, int end);
...
> > +    ExpressionNode* makeURightShiftNode(int, ExpressionNode* left, ExpressionNode* right, bool rightHasAssignments);
> 
> These should all say lineNumber. It’s not at all obvious that the integer is a line number. Also, there is an extra space on the makeDeleteNode line.

I added "lineNumber" to those methods as well as eliminated the extraneous space.
Comment 9 Michael Saboff 2011-10-31 15:13:15 PDT
Committed r98887: <http://trac.webkit.org/changeset/98887>
Comment 10 Yong Li 2012-02-17 14:04:44 PST
The patch missed the definition of VectorTraits<JSParser::Scope>, which caused problem. See Bug 78932.