[JSC] Optimize JSON.parse string fast path
Created attachment 277149 [details] Patch
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.
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?
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>*).
Committed r199968: <http://trac.webkit.org/changeset/199968>