Bug 36080 – Performance degradations when using large javascript files

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Leong]

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!

[David Leong]

Created attachment 50646 [details]
First cut at the changes

I need to fix my cygwin environment and get the create webkit patch scripts going.

[Alexey Proskuryakov]

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.

[David Leong]

Thanks Alexy, I will work on sunspider results as well as create a second bug for the script deletion timer.

[David Leong]

Created bug 36174 to address problem #1 for the short script deletion timer.

[David Leong]

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

[David Leong]

Created attachment 51342 [details]
Sunspider results with very large script file.

[David Leong]

Created attachment 51343 [details]
Test cases for sunspider. Baseline and modified with lots of text in the source code.

[David Leong]

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.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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.

[David Leong]

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

[Oliver Hunt]

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)