Bug 45289 - Refactoring multiline comments in the lexer
Summary: Refactoring multiline comments in the lexer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-07 04:47 PDT by Zoltan Herczeg
Modified: 2010-09-09 13:28 PDT (History)
4 users (show)

See Also:


Attachments
patch (4.53 KB, patch)
2010-09-07 06:05 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
Reworked patch (3.54 KB, patch)
2010-09-08 02:45 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
removing doneSemicolon label (2.36 KB, patch)
2010-09-08 04:09 PDT, Zoltan Herczeg
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 2010-09-07 04:47:04 PDT
Removing more labels.
Comment 1 Zoltan Herczeg 2010-09-07 06:05:12 PDT
Created attachment 66713 [details]
patch

--parse-only reports a big change, but I am unsure it is really caused by the patch. Who knows...

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

** TOTAL **:                   1.063x as fast    33.6ms +/- 1.5%   31.6ms +/- 1.6%     significant

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

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

  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:                   1.067x as fast     6.4ms +/- 5.8%    6.0ms +/- 0.0%     significant
    1.6.0.3:                   1.067x as fast     6.4ms +/- 5.8%    6.0ms +/- 0.0%     significant

  concat:                      1.064x as fast    16.7ms +/- 2.1%   15.7ms +/- 2.2%     significant
    jquery-mootools-prototype: 1.064x as fast    16.7ms +/- 2.1%   15.7ms +/- 2.2%     significant

SS has no change:

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

** TOTAL **:           -                 523.1ms +/- 0.6%   521.2ms +/- 0.5%

=============================================================================
Comment 2 Darin Adler 2010-09-07 14:34:10 PDT
Comment on attachment 66713 [details]
patch

> +        if (UNLIKELY(m_current == '*')) {
> +            shift();
> +            if (m_current == '/') {
> +                shift();
> +                return true;
> +            }
> +            if (m_current == '*')
> +                continue;
> +        }

This ends up comparing with '*' twice, which seems like a shame given it's also slightly confusing logic. Instead of the if statement and the continue statement, we could just use while instead of if. Does that make things slower?

> @@ -1002,7 +1027,8 @@ inNumberAfterDecimalPoint:
>          m_terminator = true;
>          if (lastTokenWasRestrKeyword()) {
>              token = SEMICOLON;
> -            goto doneSemicolon;
> +            m_delimited = true;
> +            goto returnToken;
>          }
>          goto start;
>      case CharacterInvalid:

Why is this change included in the same patch? It seems unrelated and should be done based on its own merits, not as a ridealong. We can keep the doneSemicolon symbol.
Comment 3 Zoltan Herczeg 2010-09-08 02:45:34 PDT
Created attachment 66863 [details]
Reworked patch

Just upload the final version of the patch.
Comment 4 Zoltan Herczeg 2010-09-08 03:08:08 PDT
Landed in http://trac.webkit.org/changeset/66962.
Comment 5 Zoltan Herczeg 2010-09-08 04:09:42 PDT
Created attachment 66874 [details]
removing doneSemicolon label

I still see the perf increase after apply these simple changes.

33.6ms -> 32.8ms (previous patch) -> 31.5ms (this patch)
Comment 6 Zoltan Herczeg 2010-09-08 23:54:51 PDT
Landed in http://trac.webkit.org/changeset/67066
Comment 7 WebKit Review Bot 2010-09-09 13:28:02 PDT
http://trac.webkit.org/changeset/67066 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/67065
http://trac.webkit.org/changeset/67066