Created attachment 50644 [details] Test cases We have noticed that using large javascript files results in a degradation in performance. Our use case contains a HTML page which uses with a large (>1meg) javascript file. Performance of any javascript class/function in this file is much slower than having the code split into smaller ~100k file chunks. On mobile devices we have found that that up to 70% of the time when executing javascript is doing character conversion and byteOrderMark checking. After some investigation, we found that two problems: 1) WebCore::CachedScript::script() deletes decoded data with a timer callback of 0s. If the script file is large this causes the data to be repeatedly re-parsed many times when javascript core only wants small sections of the code. 2) JSC::Lexer::setCode() is looking for the byteOrderMark for every character that javascript core is interested in. For large script files this results in many overlapping sections that have been checked before for the byte mark. We've made a simple test page to reproduce the problem. The page is based on the V8 benchmark with lots of text inserted as comments. Here are some statistics from running v8 on my desktop to showcase the problem: Tests done on a Core i7 920, 3gigs of ram, webkit trunk, running QGVLauncher and repeated several times. First, I ran the V8 test with no code changes. Next, I added lots of text into the V8 test scripts Finally, I disabled the byteOrderMark checking in JSC::Lexer and disabled the script delete timer in WebCore::CachedScript to test the performance with the workaround. V8 baseline: V8 modified: V8 modified workaround: Score: 977 806 975 Richards: 3314 2812 3279 DeltaBlue: 831 608 841 Crypto: 2211 1891 2238 RayTrace: 1193 993 1200 EarleyBoyer: 571 480 586 RegExp: 210 178 197 I am reworking the code to do a single pass BOM checking inside of cachedScript. Also, I am thinking of adding a setting to change the timeout before CachedScript class will delete the data. Any feedback and comments would be apprecitated!
Created attachment 50646 [details] First cut at the changes I need to fix my cygwin environment and get the create webkit patch scripts going.
I suggest tracking these two issues in separate bugs. When you have a patch for review, please also provide sunspider results - a reviewer will definitely want these. Although they'll probably be more interested in Mac results, Windows ones should be helpful as a first approximation.
Thanks Alexy, I will work on sunspider results as well as create a second bug for the script deletion timer.
Created bug 36174 to address problem #1 for the short script deletion timer.
Created sunspider version of the test case. (logs attached as well) In sunspider, all of the javascript code is embedded in html files. In this case, the CachedScriptSourceProvider class is not involved but StringSourceProvider is used. Moving the byte order mark checking to the string source provider also improves the performance of large script files. Here are the results: Sunspider baseline, no webkit modifications: Total: 2107 3d: 171.4 Access: 64.4 Bitops: 27.2 controlflow: 3.6 crypto: 90.4 date: 451.2 math: 70.6 regexp: 23.2 string: 1205.0 Sunspider baseline, with webkit modifications: Total: 2023.6 3d: 156.0 Access: 59.8 Bitops: 27 controlflow: 3.4 crypto: 86 date: 437.2 math: 70 regexp: 21.6 string: 1162.6 Sunspider modified with large file, no webkit modifications: Total: 2580.8 3d: 299 Access: 114.2 Bitops: 50 controlflow: 14.4 crypto: 196.8 date: 496.2 math: 90 regexp: 20.8 string: 1299.4 Sunspider modified with large file, with webkit modifications: Total: 2059.6 3d: 162.6 Access: 61.2 Bitops: 27.6 controlflow: 3.2 crypto: 87.8 date: 442.8 math: 72.6 regexp: 21.8 string: 1180.0
Created attachment 51342 [details] Sunspider results with very large script file.
Created attachment 51343 [details] Test cases for sunspider. Baseline and modified with lots of text in the source code.
Created attachment 51559 [details] First stab at a patch. This patch makes JSC::SourceProvider::hasBOMs() virtual so sub classes can inherit from it. This method is overwritten for both the CachedScriptSourceProvider and the StringSourceProvider. For the CachedScriptSourceProvider, I delegated the BOM check to the CachedScript as the buffer maybe deleted by the decoded data callback timer. For the StringSourceProvider the BOM check is done when the class is constructed. Both classes store the results of the BOM check so that repeated calls by the JSC::Parser for source code will not repeat the BOM checks. I was thinking should we move the BOM checking to the base class ScriptSourceProvider? In that case how would we deal with the decoded data deletion by the CachedScript. Any feedback or ideas would be greatly appreciated.
Attachment 51559 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/loader/CachedScript.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] WebCore/loader/CachedScript.h:33: Missing space before { [whitespace/braces] [5] WebCore/bindings/js/StringSourceProvider.h:64: Missing spaces around += [whitespace/operators] [3] WebCore/loader/CachedScript.cpp:144: Tab found; better to use spaces [whitespace/tab] [1] WebCore/loader/CachedScript.cpp:145: Missing spaces around += [whitespace/operators] [3] Total errors found: 5 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 51559 [details] First stab at a patch. Please fix the style errors and be sure to include the bug URL in your ChangeLog entry.
From Oliver's comment in the other error: https://bugs.webkit.org/show_bug.cgi?id=36174 "Can you re-measure following the recent parser + lexer + bom handling changes? That said the 0-delay timer did seem low to me when i was doing the bom handling stuff." Updated the results with the latest webkit baseline r64659. Looks like performance degradations are now much smaller <4% difference. We can probably close this bug. V8 Baseline 966 978 981 Richards 3115 3098 3154 DeltaBlue 808 829 823 Cryoto 2352 2369 2345 RayTrace 855 886 909 EarleyBoyer 592 594 593 Regexp 272 273 272 V8 Modified 958 953 949 Richards 3091 3140 2993 DeltaBlue 814 810 800 Cryoto 2323 2311 2320 RayTrace 885 886 899 EarleyBoyer 599 589 581 Regexp 250 245 252 SunSpider 1552.4 1633.8 1637.2 3d 151.6 138.4 144.0 access 61.4 65.4 69.4 bitops 27.8 27.6 27.2 controlflow 3.4 4.0 4.0 crypto 60.6 62.8 65.6 date 319.4 343.2 337.4 math 37.6 37.0 37.8 regexp 17.4 17.6 17.8 string 873.2 937.8 934.0 SunSpider-slow 1705.8 1728.4 1701.6 3d 165.0 161.6 146.6 access 66.6 71.2 67.0 bitops 27.8 29 27.2 controlflow 3.4 3.8 3.4 crypto 70.0 71.6 70.0 date 355.2 358.8 351.8 math 37.2 37.0 36.8 regexp 17.6 17.2 17.6 string 963.0 978.2 981.2
Closing per above comment. In all likelihood this has ceased to be a problem due to the update to modern es5 semantics (which get rid of the bom stripping semantics)