Bug 156953 – [JSC] Optimize JSON.parse string fast path

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Optimize JSON.parse string fast path

[Yusuke Suzuki]

Created attachment 277149 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 277149 [details]
Patch

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

Add comments.

> Source/JavaScriptCore/runtime/LiteralParser.h:74
> +

Convert LiteralParserToken to noncopyable class.

> Source/JavaScriptCore/runtime/LiteralParser.h:156
> +        StringBuilder m_builder;

Now, token's string's ownership is maintained by this builder.

[Mark Lam]

Comment on attachment 277149 [details]
Patch

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

r=me with suggestion.  Nice work on improving kraken.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:717
> -                        LiteralParserToken<CharType> stringToken = m_lexer.currentToken();
> -                        m_lexer.next();
> +                        const LiteralParserToken<CharType>& stringToken = m_lexer.currentToken();
>                          if (stringToken.stringIs8Bit)
>                              lastValue = jsString(m_exec, makeIdentifier(stringToken.stringToken8, stringToken.stringLength).string());
>                          else
>                              lastValue = jsString(m_exec, makeIdentifier(stringToken.stringToken16, stringToken.stringLength).string());
> +                        m_lexer.next();

I understand that the reading of the token values needs be done before the call to m_lexer.next() because the lexer could alter the contents of the underlying StringBuilder that backs the token string in next().

My concern is: from reading this code (without digging deeper into the implementation of the lexer), it is not obvious that the validity of the current token is dependent on the internal state of the lexer.  It would be nice if there's some way to assert that the token string we're consuming is the one from the current token.  The only idea I can come up with for doing that assert is to assign some sort of a token ID to the token.  For example:

    template<CharType>
    class LiteralParserTokenPtr {
        LiteralParserTokenPtr(Lexer<CharType>& lexer)
            : m_lexer(lexer)
#if !ASSERTS_DISABLED
            , m_tokenID(lexer.currentTokenID)
#endif
        { }

        ALWAYS_INLINE const LiteralParserToken<CharType>* operator->() const
        {
            ASSERT(m_tokenID == m_lexer.currentTokenID);
            return &m_lexer.m_currentToken;
        }

    private:
        Lexer<CharType>& m_lexer;
#if !ASSERTS_DISABLED
        unsigned m_tokenID;
#endif
    }

    // In Lexer:
    friend class LiteralParserTokenPtr<CharType>;

    LiteralParserTokenPtr<CharType>& currentToken()
    {
        return LiteralParserTokenPtr<CharType>(*this);
    }
    ...
#if !ASSERTS_DISABLED
    unsigned m_currentTokenID { 0 };
#endif

    // In Lexer::next()
    if (!ASSERTS_DISABLED)
        m_currentTokenID++;
    ...

    // When parsing:
    LiteralParserTokenPtr<CharType> token = m_lexer.currentToken();
    lastValue = ... token->stringToken8 ...


This way, we will know if anyone ever tries to use the token after calling lexer.next().  What do you think?

[Yusuke Suzuki]

Comment on attachment 277149 [details]
Patch

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

>> Source/JavaScriptCore/runtime/LiteralParser.cpp:717
>> +                        m_lexer.next();
> 
> I understand that the reading of the token values needs be done before the call to m_lexer.next() because the lexer could alter the contents of the underlying StringBuilder that backs the token string in next().
> 
> My concern is: from reading this code (without digging deeper into the implementation of the lexer), it is not obvious that the validity of the current token is dependent on the internal state of the lexer.  It would be nice if there's some way to assert that the token string we're consuming is the one from the current token.  The only idea I can come up with for doing that assert is to assign some sort of a token ID to the token.  For example:
> 
>     template<CharType>
>     class LiteralParserTokenPtr {
>         LiteralParserTokenPtr(Lexer<CharType>& lexer)
>             : m_lexer(lexer)
> #if !ASSERTS_DISABLED
>             , m_tokenID(lexer.currentTokenID)
> #endif
>         { }
> 
>         ALWAYS_INLINE const LiteralParserToken<CharType>* operator->() const
>         {
>             ASSERT(m_tokenID == m_lexer.currentTokenID);
>             return &m_lexer.m_currentToken;
>         }
> 
>     private:
>         Lexer<CharType>& m_lexer;
> #if !ASSERTS_DISABLED
>         unsigned m_tokenID;
> #endif
>     }
> 
>     // In Lexer:
>     friend class LiteralParserTokenPtr<CharType>;
> 
>     LiteralParserTokenPtr<CharType>& currentToken()
>     {
>         return LiteralParserTokenPtr<CharType>(*this);
>     }
>     ...
> #if !ASSERTS_DISABLED
>     unsigned m_currentTokenID { 0 };
> #endif
> 
>     // In Lexer::next()
>     if (!ASSERTS_DISABLED)
>         m_currentTokenID++;
>     ...
> 
>     // When parsing:
>     LiteralParserTokenPtr<CharType> token = m_lexer.currentToken();
>     lastValue = ... token->stringToken8 ...
> 
> 
> This way, we will know if anyone ever tries to use the token after calling lexer.next().  What do you think?

Your suggestion looks nice.

I think switching this with the raw pointer is preferable.

Under debug mode, we use the above adapter. And under the release mode, we use the raw pointer (LiteralParserToken<CharType>*).

[Yusuke Suzuki]

Committed r199968: <http://trac.webkit.org/changeset/199968>