| Summary: | Optimize String::fromUTF8 for ASCII | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||
| Component: | Web Template Framework | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, cmarcelo, commit-queue, rniwa | ||||
| Priority: | P2 | Keywords: | BlinkMergeCandidate | ||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Gyuyoung Kim
2013-06-13 03:23:44 PDT
Created attachment 204566 [details]
Patch
Comment on attachment 204566 [details]
Patch
Looks good. Thanks for porting this over!
Comment on attachment 204566 [details] Patch Clearing flags on attachment: 204566 Committed r151556: <http://trac.webkit.org/changeset/151556> All reviewed patches have been landed. Closing bug. 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). (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. (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. (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. 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. (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. (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. Sounds good to me then. Thank you for getting the numbers. |