Current implementation of Parser::createSavePoint is a foot gun

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

We often create a SavePoint, and restore it, when we really want to do more.
Often we want to save and restore some internal fields and not just lexer state.

[Oliver Hunt]

SavePoint should be saving everything necessary - I'm not saying it is, i'm saying that the whole point of it is to do that. SavePoint should not be a footgun, it should just be "correct"

The correct solution is probably to put all parser state, and all lever state, into structs so that SavePoint can just do a struct copy and not worry about things going out of sync.

[Saam Barati]

Created attachment 269400 [details]
patch

[Saam Barati]

(In reply to comment #1)
> SavePoint should be saving everything necessary - I'm not saying it is, i'm
> saying that the whole point of it is to do that. SavePoint should not be a
> footgun, it should just be "correct"
I agree. SavePoint should just work. I've renamed the old SavePoint
to LexerState and made the new SavePoint LexerState+ParserState.

[Saam Barati]

(In reply to comment #1)

> The correct solution is probably to put all parser state, and all lever
> state, into structs so that SavePoint can just do a struct copy and not
> worry about things going out of sync.
That's a good idea. I'll change it so that Parser now owns
a ParserState. It might not be as clean to have LexerState
be this way.

[Saam Barati]

Created attachment 269402 [details]
patch

[Saam Barati]

Created attachment 269403 [details]
patch

renamed the bug

[Oliver Hunt]

Comment on attachment 269403 [details]
patch

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

> Source/JavaScriptCore/parser/Parser.h:1302
> +    ALWAYS_INLINE LexerState saveLexerState()

An idea: make this internalSaveLexerState, make it private, make SavePoint a friend of lexer, and have SavePoint have a saveLexerState(Lexer*) function that calls it.

But that's a somewhat convoluted so maybe not worth it?

> Source/JavaScriptCore/parser/Parser.h:1368
>      const Identifier* m_lastIdentifier;
>      const Identifier* m_lastFunctionName;

We should probably pull all of these into parser state. What's the rational for not doing so?

[Saam Barati]

Comment on attachment 269403 [details]
patch

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

>> Source/JavaScriptCore/parser/Parser.h:1302
>> +    ALWAYS_INLINE LexerState saveLexerState()
> 
> An idea: make this internalSaveLexerState, make it private, make SavePoint a friend of lexer, and have SavePoint have a saveLexerState(Lexer*) function that calls it.
> 
> But that's a somewhat convoluted so maybe not worth it?

Ok. The reason I did it this way is that there are a few places where we only need to 
save lexer information (i.e, one token lookahead).
That said, I think it's still somewhat of a footgun to have this. Let me see
what the performance is if we always use SavePoints.

>> Source/JavaScriptCore/parser/Parser.h:1368
>>      const Identifier* m_lastFunctionName;
> 
> We should probably pull all of these into parser state. What's the rational for not doing so?

I thought about this, and I think for correctness/completeness, we should have it.
We also might want m_statementDepth there, too, even though we sometimes 
manage manually with DepthManager.

[Saam Barati]

It's perf neutral to always use SavePoints.
Going to do that.
I'm going to rename saveLexerState to internalSaveLexerState, but
I'm going to keep it a Parser method because it consults Parser's fields.

[Saam Barati]

Created attachment 269594 [details]
patch

[WebKit Commit Bot]

Comment on attachment 269594 [details]
patch

Clearing flags on attachment: 269594

Committed r195484: <http://trac.webkit.org/changeset/195484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This regressed Dromaeo CSS selector tests by 2.5%.

[Saam Barati]

(In reply to comment #13)
> This regressed Dromaeo CSS selector tests by 2.5%.

Writing this for the record.

This patch didn't actually regress Dromaeo. Our calculations
of performance showed a regression because this patch fixed a 
bug where one Dromaeo test wouldn't parse. Because it now passes
with this patch, we showed a regression.