* SUMMARY JavaScriptCore should parse sourceURL and sourceMappingURL directives. Currently Web Inspector runs 1-2 regexes over every script source to scan for "sourceURL" and "sourceMappingURL" directives. This is rather inefficient, and just sampling a few websites (apple, verge, cnn) my Mac Pro saw 100+ scripts required >1ms scans. Durations(us) value ------------- Distribution ------------- count 8 | 0 16 |@@@@@@@@@@@@@@@@@@@@@@@@@@ 2598 32 |@@@@@@@@ 787 64 |@@ 205 128 |@ 60 256 |@@ 178 512 |@ 95 1024 |@ 61 2048 | 33 4096 | 6 8192 | 0 Though the majority of scans are short, it should be possible to detect these comment directives while parsing script comments and eliminate these scans entirely. * NOTES - The rules for parsing sourceURL and sourceMappingURL directives are somewhat defined by older articles: > https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.lmz475t4mvbx > http://www.softwareishard.com/blog/firebug/firebug-tip-label-dynamic-scripts-with-sourceurl-directive/ > Syntax of the directive needs to match this regular expression: > /\/\/[@#]\ssourceURL=\s*(\S*?)\s*$/m;
<rdar://problem/23095020>
I have implemented this, I just need to write tests.
Created attachment 263027 [details] [PATCH] Proposed Fix
Comment on attachment 263027 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=263027&action=review > Source/JavaScriptCore/parser/Lexer.cpp:2269 > + skipWhitespace(); Oop, I should drop this skipWhitespace, as it looks like our old code (and the documentation I linked to) only expects a single whitespace character between "//#" and the directive name.
Created attachment 263029 [details] [PATCH] Proposed Fix
Comment on attachment 263029 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=263029&action=review > Source/JavaScriptCore/parser/Lexer.cpp:1750 > + append8(stringStart, stringEnd - stringStart); > + return String(m_buffer8.data(), m_buffer8.size()); Note to reviewers. It is possible that the sourceMappingURL is a long dataURL. Here we are making a copy, which matches what was done previously, but perhaps there is a better solution that avoids a copy. I'm open the suggestions.
Comment on attachment 263029 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=263029&action=review r=me >> Source/JavaScriptCore/parser/Lexer.cpp:1750 >> + return String(m_buffer8.data(), m_buffer8.size()); > > Note to reviewers. It is possible that the sourceMappingURL is a long dataURL. Here we are making a copy, which matches what was done previously, but perhaps there is a better solution that avoids a copy. I'm open the suggestions. This is how we do other strings in the parser. The cost can never exceed the original cost of decoding the input script, so it's probably OK. Long-term, perhaps the parser should change to use substrings of the original string more aggressively. > Source/JavaScriptCore/parser/Lexer.cpp:1757 > + unsigned lengthToCheck = length - 1; // Ignore the ending NUL byte in the string literal. NULL > Source/JavaScriptCore/parser/Lexer.h:215 > + String m_sourceURL; > + String m_sourceMappingURL; These names feel a bit too on point. How about "m_sourceURLDirective" or something to help show that this is not "the truth" but just a developer setting? > Source/JavaScriptCore/parser/SourceProvider.h:78 > + void setSourceURL(const String& sourceURL) { m_sourceURL = sourceURL; } > + void setSourceMappingURL(const String& sourceMappingURL) { m_sourceMappingURL = sourceMappingURL; } > > JS_EXPORT_PRIVATE void getID(); > Vector<size_t>& lineStarts(); > > String m_url; > + String m_sourceURL; > + String m_sourceMappingURL; See above.
Part of why this is slow is that we use the regex interpret instead of the JIT. That's fixable -- but probably the best long term solution is to update the CSS parser and stop scanning at all.
Comment on attachment 263029 [details] [PATCH] Proposed Fix Clearing flags on attachment: 263029 Committed r191135: <http://trac.webkit.org/changeset/191135>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 150197
Comment on attachment 263029 [details] [PATCH] Proposed Fix Attachment 263029 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/312781 Number of test failures exceeded the failure limit.
Created attachment 263607 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
When reading the next token, the Lexer asserts the buffers are empty. So we need to clear the buffer after we use it. Other places do similar work (shrinking the buffer). Since this is straight forward I'm not going to ask of another review! diff --git a/Source/JavaScriptCore/parser/Lexer.cpp b/Source/JavaScriptCore/parser/Lexer.cpp index bf13d74..46d0ca1 100644 --- a/Source/JavaScriptCore/parser/Lexer.cpp +++ b/Source/JavaScriptCore/parser/Lexer.cpp @@ -1747,7 +1747,9 @@ ALWAYS_INLINE String Lexer<T>::parseCommentDirectiveValue() return String(); append8(stringStart, stringEnd - stringStart); - return String(m_buffer8.data(), m_buffer8.size()); + String result = String(m_buffer8.data(), m_buffer8.size()); + m_buffer8.shrink(0); + return result; }
Remanded with ASSERT fix: <http://trac.webkit.org/changeset/191355>