Bug 117586 - Optimize String::fromUTF8 for ASCII
Summary: Optimize String::fromUTF8 for ASCII
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks:
 
Reported: 2013-06-13 03:23 PDT by Gyuyoung Kim
Modified: 2013-06-22 19:48 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.01 KB, patch)
2013-06-13 03:26 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2013-06-13 03:23:44 PDT
We probably need to consider to merge with https://src.chromium.org/viewvc/blink?view=rev&revision=152243. Current String::fromUTF8() implementation converts 8 bit ASCII character into 16 bit. Instead of always trying to convert into a 16 bit buffer, we can add a call to charactersAreAllASCII. In the common case when characters are ASCII, we directly copy it into an 8 bit string buffer.
Comment 1 Gyuyoung Kim 2013-06-13 03:26:38 PDT
Created attachment 204566 [details]
Patch
Comment 2 Brent Fulgham 2013-06-13 09:52:40 PDT
Comment on attachment 204566 [details]
Patch

Looks good.  Thanks for porting this over!
Comment 3 WebKit Commit Bot 2013-06-13 09:56:19 PDT
Comment on attachment 204566 [details]
Patch

Clearing flags on attachment: 204566

Committed r151556: <http://trac.webkit.org/changeset/151556>
Comment 4 WebKit Commit Bot 2013-06-13 09:56:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Benjamin Poulain 2013-06-16 19:36:10 PDT
We looked into this change but I was unconvinced.

Did you get actual performance numbers? If not, please get numbers and follow up (or revert).
Comment 6 Gyuyoung Kim 2013-06-16 20:12:22 PDT
(In reply to comment #5)
> We looked into this change but I was unconvinced.
> 
> Did you get actual performance numbers? If not, please get numbers and follow up (or revert).

It looks Adam Barth didn't get meaning performance number. Anyway, I will try to get performance number. If failed, let's consider to revert this patch.
Comment 7 Ryosuke Niwa 2013-06-20 00:21:28 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > We looked into this change but I was unconvinced.
> > 
> > Did you get actual performance numbers? If not, please get numbers and follow up (or revert).
> 
> It looks Adam Barth didn't get meaning performance number. Anyway, I will try to get performance number. If failed, let's consider to revert this patch.

Did you get a number?  I did look into this but it was an overall perf. regression.

I'd like to know on which test you saw a performance improvement.
Comment 8 Gyuyoung Kim 2013-06-20 00:44:29 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > We looked into this change but I was unconvinced.
> > > 
> > > Did you get actual performance numbers? If not, please get numbers and follow up (or revert).
> > 
> > It looks Adam Barth didn't get meaning performance number. Anyway, I will try to get performance number. If failed, let's consider to revert this patch.
> 
> Did you get a number?  I did look into this but it was an overall perf. regression.
> 
> I'd like to know on which test you saw a performance improvement.

Sorry. I don't get meaningful number on this patch yet. If there is a performance regression after this patch, I don't mind to roll this patch out for now. When I get a number or better number, I will pong you guys.
Comment 9 Gyuyoung Kim 2013-06-20 03:43:58 PDT
I simply run sunspider test w/ and w/o this patch. However, I fail to get a meaningful difference between them. String tests of sunspider took 45 ms ~ 47 ms generally both of them.

string : 47.1 ms
 - base64: 3.7 ms
 - fasta: 8.4 ms
 - tagcloud: 9.2 ms
 - unpack-code: 20.5 ms
 - validate-input: 5.3 ms

I'm not sure if sunspider is correct tool for this patch. However, it shows this patch doesn't improve js string performance at least. I will try to get a number a little further.
Comment 10 Benjamin Poulain 2013-06-20 09:07:45 PDT
(In reply to comment #9)
> I'm not sure if sunspider is correct tool for this patch. However, it shows this patch doesn't improve js string performance at least. I will try to get a number a little further.

I think a good test would be checking how much time we spend in this function when loading a non-latin1 website.

For example, take a well known Korean website, can it. Load the canned version with and without the patch and measure the function time with DTrace.
Comment 11 Gyuyoung Kim 2013-06-21 01:43:42 PDT
(In reply to comment #10)
 
> For example, take a well known Korean website, can it. Load the canned version with and without the patch and measure the function time with DTrace.

I measure total execution time on String::fromUTF8() using "struct timeval". I used popular korean web sites (http://www.naver.com and http://www.daum.net)

I measure the execution time with entry point and entrance point. I calculated the time gap using both of them. Result below shows total execution time in fromUTF8() until finishing front page loading. 


When using this patch,
1. naver : 250 us ~ 300 us
2. daum : 200 us ~ 250 us

When using previous one,
1. naver : 1,160 us ~ 1,300 us
2. daum : 800 us ~ 900 us

According to this profiling, this patch looks reduce execution time by about 2 ~ 3 times.

If there is any problems or mistakes in my test, please let me know.
Comment 12 Benjamin Poulain 2013-06-22 19:48:25 PDT
Sounds good to me then.

Thank you for getting the numbers.