Bug 41606 - Refactored string parsing in the lexer
Summary: Refactored string parsing in the lexer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-05 04:55 PDT by Zoltan Herczeg
Modified: 2010-07-07 00:06 PDT (History)
2 users (show)

See Also:


Attachments
patch (10.57 KB, patch)
2010-07-05 04:59 PDT, Zoltan Herczeg
oliver: review+
Details | Formatted Diff | Diff
Lexer.cpp.gcov (49.25 KB, text/plain)
2010-07-05 05:03 PDT, Zoltan Herczeg
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 2010-07-05 04:55:13 PDT
The current sunspider parse-only results are the following:

=============================================================================

** TOTAL **:                   1.044x as fast    33.1ms +/- 1.9%   31.7ms +/- 1.1%     significant

=============================================================================

  jquery:                      -                  5.1ms +/- 4.4%    5.1ms +/- 4.4%
    1.3.2:                     -                  5.1ms +/- 4.4%    5.1ms +/- 4.4%

  mootools:                    1.080x as fast     5.4ms +/- 6.8%    5.0ms +/- 0.0%     significant
    1.2.2-core-nc:             1.080x as fast     5.4ms +/- 6.8%    5.0ms +/- 0.0%     significant

  prototype:                   -                  6.1ms +/- 3.7%    6.0ms +/- 0.0%
    1.6.0.3:                   -                  6.1ms +/- 3.7%    6.0ms +/- 0.0%

  concat:                      1.058x as fast    16.5ms +/- 2.3%   15.6ms +/- 2.4%     significant
    jquery-mootools-prototype: 1.058x as fast    16.5ms +/- 2.3%   15.6ms +/- 2.4%     significant

But it was not before, actually it was a 5% slowdown. This patch was actually completed a week ago, and I didn't really changed it. I planned to submit a patch with a gcov output and ask for help. (I usually run sunspider 10-15 times before choose the best measurement, to lessen the effect of deviation caused by the OS. So this patch was really slower before, and faster now, albeit I think it is not the effect of the patch, more likely the changed binary structure) Anyway, the gcov output is also attached.
Comment 1 Zoltan Herczeg 2010-07-05 04:59:43 PDT
Created attachment 60514 [details]
patch
Comment 2 WebKit Review Bot 2010-07-05 05:01:46 PDT
Attachment 60514 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/parser/Lexer.cpp:398:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zoltan Herczeg 2010-07-05 05:03:46 PDT
Created attachment 60515 [details]
Lexer.cpp.gcov
Comment 4 Zoltan Herczeg 2010-07-05 05:05:38 PDT
The gcov profile is for concat-jquery-mootools-prototype
Comment 5 Zoltan Herczeg 2010-07-05 06:40:05 PDT
Hm, maybe I have an idea what is causing the slowdown. The original code, there was two makeIdentifier call. I did a few changes before, and realized I removed one of them (one was for a fast path when the m_buffer was not needed). I put it back (thought it was a stupid move), and the slowdown appeared again. Later I refactored the code, that even though I keep the fast case, only one makeIdentifier appeared in the code, and the speed was nearly as fast as the patch added to this bug. Maybe the ALWAYS_INLINE makeIdentifier is more computation expensive than I expected... at least on my machine.
Comment 6 Oliver Hunt 2010-07-06 23:32:31 PDT
Comment on attachment 60514 [details]
patch

r=me
Comment 7 Zoltan Herczeg 2010-07-07 00:06:14 PDT
Landed in http://trac.webkit.org/changeset/62628
Closing bug