Bug 36174 – Short CachedScript decoded data timer causes performance degradations when using large javascript files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[David Leong]

This bug is a branch off from bug 36080. 

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.

This bug is to address part 1) where the script deletion timer is too short. Bug 36080 will address part 2) where the BOM checking is done.

[Andre Pedralho]

Adding some guys in the CC list that can be interested on solving it.

[David Leong]

Created attachment 57614 [details]
First cut at the changes

Added new API in QWebSettings to change the CachedScript timer
Added new logic and API to CachedScript class to change how long before decoded data is deleted.

[Benjamin Poulain]

I don't know the CachedScript well, but it sounds like a solution is to store the decoded script in the cache.
Would it be possible to cache the decoded string?

[David Leong]

The decoded data is cached in the CachedScript class already. The question is when to delete the decoded data. The current behavior is to delete it right away with a 0-time timer callback.

It would be great to have a configurable option to delay the deletion of the decoded data for short term performance reasons while keeping memory use low in the long run. 

For example what is happening now:

Some page with lots of buttons driven mostly by Javascript:
-User clicks on a button
-JavaScriptCore requests for the script from CachedScript
-Script is decoded, saved and deletion timer is kicked off
-JavaScriptCore does some parsing, execution etc.
-Deletion timer fires, data deleted
-JavaScriptCore wants the same data again (seems like it requests the data for every function it parses?)
-Script is decoded again, saved and deletion timer kicked off
-Javascript core does some parsing, execution etc.
-And this repeats...

[Benjamin Poulain]

(In reply to comment #4)
> The decoded data is cached in the CachedScript class already. The question is when to delete the decoded data. The current behavior is to delete it right away with a 0-time timer callback.

I know CachedScript decode the data. I wondered about storing decoded data in the _cache_. That would make it faster everytime, not just if you are lucky and ask the data before  a timeout.

[David Leong]

Did some more digging into this.

Most sites are returning UTF-8 encoded JS files, so if we keep the decoded version around forever it would double the memory use permanently for scripts. 
(Sites like engadget, wikipedia etc)

Right now we are storing the plain UTF-8 version and then keeping the decoded unicode version for very short time. The peak usage would be ~3x of keeping only the unicode version around.

I tried de-referencing the utf-8 version and just kept the decoded version around. Everything seems to work OK still.

[Alexey Proskuryakov]

Please note that a single resource can be decoded differently (see bug 22952). This doesn't currently work in WebKit anyway, but that's one of the reasons to keep original data.

But even doubling the amount of data kept by CachedResources would have a negative effect on memory consumption.

I don't feel very good about this patch - the choice between wasting memory and bad performance is a poor choice to ask embedders to make. But I'm not formally reviewing it, as people who actually worked on this aspect of performance are better qualified to.

[Oliver Hunt]

Comment on attachment 57614 [details]
First cut at the changes

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.

[David Leong]

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

[Ariya Hidayat]

David, do we still want to close this bug or not?

[Eric Seidel (no email)]

Comment on attachment 57614 [details]
First cut at the changes

This is some strange extra TTL control.  Seems this could be integrated in other ways.

Given the mild excitement in the bug for this added API/complication, I think we should just r- this.

[David Leong]

Yes Ariya, please close the error. 

The latest webkit.org trunk does not suffer from this problem.

[Adam Barth]

Okiedokes.