Created attachment 79268 [details] experimental patch

Comment on attachment 79268 [details] experimental patch View in context: https://bugs.webkit.org/attachment.cgi?id=79268&action=review > Source/JavaScriptCore/parser/JSParser.cpp:121 > + struct CachedFunctionInfo : public SourceProvider::CachedSourceInfo { > + CachedFunctionInfo(JSToken closeBraceToken) > + : closeBraceToken(closeBraceToken) > + { > + } > + unsigned approximateByteSize() const > + { > + static const unsigned assummedAverageIdentifierSize = 20; > + unsigned size = sizeof(*this); > + size += declaredVariables.size() * assummedAverageIdentifierSize; > + size += usedVariables.size() * assummedAverageIdentifierSize; > + size += closedVariables.size() * assummedAverageIdentifierSize; > + size += writtenVariables.size() * assummedAverageIdentifierSize; > + return size; > + } > + > + JSToken closeBraceToken; > + bool usesEval; > + Vector<RefPtr<StringImpl> > declaredVariables; > + Vector<RefPtr<StringImpl> > usedVariables; > + Vector<RefPtr<StringImpl> > closedVariables; > + Vector<RefPtr<StringImpl> > writtenVariables; > + }; The cached info doesn't need to store a functions declared or closed variables. All it should need to store is the used and written free variables which is basically usedVariables - declaredVariables and writtenVariables - declaredVariables. Rather than storing the entirety of the close brace token you should be able to simply store the start character and line number. > Source/JavaScriptCore/parser/JSParser.cpp:1326 > + if (CachedFunctionInfo* cachedInfo = findCachedFunctionInfo(openBracePos)) { > + // If we know about this function already, we can use the cached info and skip the parser to the end of the function. > + body = context.createFunctionBody(strictMode()); > + > + functionScope->restoreFunctionInfo(cachedInfo); > + failIfFalse(popScope(functionScope, TreeBuilder::NeedsFreeVariableInfo)); > + > + m_token = cachedInfo->closeBraceToken; > + m_lexer->setOffsetAfterCloseBraceToken(m_token); > + closeBracePos = m_token.m_info.startOffset; > + > + next(); > + return true; > + } The ability to use the cache is should be determined by the type of treebuilder being used -- something like TreeBuilder::CanUseFunctionCache will be fine.

Created attachment 79355 [details] patch for review Function cache size seems to be around 10% of the source size on average. Loading wsj.com the patch avoids parsing ~0.5M characters worth of js on the first load and ~1.5M when revisiting the site.

Attachment 79355 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/parser/SourceProvider.h:60: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 79405 [details] better size calculation and stylistic improvements

Attachment 79405 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/parser/SourceProvider.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/loader/cache/CachedScript.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 79410 [details] use shrinkToFit

Attachment 79410 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/parser/SourceProvider.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/loader/cache/CachedScript.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 79410 [details] use shrinkToFit View in context: https://bugs.webkit.org/attachment.cgi?id=79410&action=review r- due to comments > Source/JavaScriptCore/parser/Lexer.h:88 > + ASSERT(token.m_type == CLOSEBRACE); > + m_lineNumber = token.m_info.line; > + setOffset(token.m_info.endOffset); > + if (UNLIKELY(m_code == m_codeEnd)) > + m_current = -1; > + m_delimited = true; > + m_lastToken = CLOSEBRACE; this should be a call to setOffset, although you'll need to transfer if (UNLIKELY(m_code == m_codeEnd)) m_current = -1; To setOffset as well. > Source/JavaScriptCore/parser/SourceProvider.h:76 > + I think we really want SourceProviderCache to hang off of SourceProvider itself. It will allow it to be devirtualised sensibly.

Created attachment 79506 [details] fixes based on oliver's comments - create a cache for all SourceProviders, not just those connected to WebCore cache - use setOffset/setLineNumber to restore lexer state

Comment on attachment 79506 [details] fixes based on oliver's comments r=me

This is leaky: bug 53061.