Summary: | Cache function offsets to speed up javascript parsing | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, ggaren, laszlo.gombos, oliver, simon.fraser, webkit.review.bot, zalan | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Bug Depends on: | 52814 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Antti Koivisto
2011-01-18 05:35:23 PST
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. |