Bug 117586 – Optimize String::fromUTF8 for ASCII

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 204566 [details]
Patch

[Brent Fulgham]

Comment on attachment 204566 [details]
Patch

Looks good.  Thanks for porting this over!

[WebKit Commit Bot]

Comment on attachment 204566 [details]
Patch

Clearing flags on attachment: 204566

Committed r151556: <http://trac.webkit.org/changeset/151556>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

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).

[Gyuyoung Kim]

(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.

[Ryosuke Niwa]

(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.

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

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.

[Benjamin Poulain]

(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.

[Gyuyoung Kim]

(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.

[Benjamin Poulain]

Sounds good to me then.

Thank you for getting the numbers.