507526 - use getc_unlocked in the scanner rather than getc

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

** TOTAL **:           1.041x as fast    1020.1ms +/- 0.2%   979.6ms +/- 0.3%     significant

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

  3d:                  1.010x as fast     141.4ms +/- 0.8%   140.0ms +/- 0.3%     significant
    cube:              -                   40.0ms +/- 1.2%    39.4ms +/- 1.3% 
    morph:             *1.039x as slow*    28.5ms +/- 1.3%    29.6ms +/- 1.2%     significant
    raytrace:          1.027x as fast      72.9ms +/- 1.2%    71.0ms +/- 0.5%     significant

  access:              1.008x as fast     142.0ms +/- 0.5%   140.9ms +/- 0.8%     significant
    binary-trees:      1.014x as fast      43.7ms +/- 0.8%    43.1ms +/- 0.5%     significant
    fannkuch:          -                   57.9ms +/- 0.7%    57.5ms +/- 1.6% 
    nbody:             -                   26.3ms +/- 1.3%    26.2ms +/- 1.2% 
    nsieve:            -                   14.1ms +/- 1.6%    14.1ms +/- 1.6% 

  bitops:              -                   35.7ms +/- 1.6%    35.4ms +/- 1.4% 
    3bit-bits-in-byte: -                    1.7ms +/- 20.3%     1.5ms +/- 25.1% 
    bits-in-byte:      -                    8.3ms +/- 4.2%     8.3ms +/- 4.2% 
    bitwise-and:       ??                   2.2ms +/- 13.7%     2.3ms +/- 15.0%     not conclusive: might be *1.045x as slow*
    nsieve-bits:       -                   23.5ms +/- 1.6%    23.3ms +/- 1.5% 

  controlflow:         -                   33.7ms +/- 1.0%    33.4ms +/- 1.1% 
    recursive:         -                   33.7ms +/- 1.0%    33.4ms +/- 1.1% 

  crypto:              1.050x as fast      56.6ms +/- 1.7%    53.9ms +/- 2.2%     significant
    aes:               1.043x as fast      31.8ms +/- 2.3%    30.5ms +/- 3.0%     significant
    md5:               1.067x as fast      15.9ms +/- 2.6%    14.9ms +/- 1.5%     significant
    sha1:              1.047x as fast       8.9ms +/- 2.5%     8.5ms +/- 4.4%     significant

  date:                ??                 151.2ms +/- 0.4%   151.7ms +/- 0.4%     not conclusive: might be *1.003x as slow*
    format-tofte:      *1.012x as slow*    69.2ms +/- 0.7%    70.0ms +/- 0.7%     significant
    format-xparb:      -                   82.0ms +/- 0.6%    81.7ms +/- 0.4% 

  math:                -                   30.6ms +/- 2.0%    30.0ms +/- 1.9% 
    cordic:            1.056x as fast      11.3ms +/- 4.3%    10.7ms +/- 3.2%     significant
    partial-sums:      *1.008x as slow*    13.0ms +/- 0.0%    13.1ms +/- 1.7%     significant
    spectral-norm:     -                    6.3ms +/- 5.5%     6.2ms +/- 4.9% 

  regexp:              1.21x as fast       49.4ms +/- 0.7%    40.9ms +/- 0.6%     significant
    dna:               1.21x as fast       49.4ms +/- 0.7%    40.9ms +/- 0.6%     significant

  string:              1.074x as fast     379.5ms +/- 0.2%   353.4ms +/- 0.2%     significant
    base64:            -                   20.9ms +/- 1.1%    20.8ms +/- 1.4% 
    fasta:             -                   73.3ms +/- 0.7%    72.8ms +/- 0.6% 
    tagcloud:          1.131x as fast     114.6ms +/- 0.3%   101.3ms +/- 0.5%     significant
    unpack-code:       1.100x as fast     128.1ms +/- 0.4%   116.5ms +/- 0.3%     significant
    validate-input:    -                   42.6ms +/- 1.6%    42.0ms +/- 0.8%

Comment 1

[Robert Sayre]

Attachment: getc_unlocked

Comment 2

[Andreas Gal :gal]

Just for fun, should we try mmap?

Comment 3

[Andreas Gal :gal]

Comment on attachment 391755 [details] [diff] [review]
getc_unlocked

Does this work with all platforms?

Comment 4

[Robert Sayre]

http://msdn.microsoft.com/en-us/library/sbc3xkz4.aspx
http://www.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html

I think it should...

Comment 5

[Robert Sayre]

Attachment: _get_nolock for windows

this works on everything except for Windows Mobile

Comment 6

[Robert Sayre]

Attachment: works everywhere

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 392215 [details] [diff] [review]
works everywhere

Doesn't MSVC warn if you re#define a macro that was already defined?  If so, avoid it with #ifdef WINCE/#else/#endif.

Comment 8

[Robert Sayre]

looks like we use gcc on WinMo, but there we have it:

jsscan.cpp
e:/builds/slave/winmo-hg/build/js/src/jsscan.cpp(267) : warning C4005: 'getc_unlocked' : macro redefinition
        e:/builds/slave/winmo-hg/build/js/src/jsscan.cpp(265) : see previous definition of 'getc_unlocked'
jsscope.cpp

Comment 9

[Brendan Eich [:brendan]]

Always #undef before re-#define-ing?

/be

Comment 10

[Jacek Caban]

Attachment: mingw fix

This patch broke mingw build that doesn't support get_unlocked nor _getc_nolock. I've attached a fix that. I don't have setup to test WinCE build. Another solution would be changing that ifdef to:
#if defined(WINCE) || defined(__GNUC__)
but it's useless as gcc is used for WinCE anyway.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We build WinCE with MSVC, actually.

Comment 12

[Brendan Eich [:brendan]]

Shouldn't we use an autoconf test or something other than a tangle of compiler and OS ifdefs?

/be

Comment 13

[Roy Tam]

changeset b211fcf538a9 breaks build using MSVC 7.1.

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b211fcf538a9

Comment 15

[Jacek Caban]

(In reply to comment #11)
> We build WinCE with MSVC, actually.

Then my patch is right although my comment was wrong (I messed the patch I've attached with the alternative one, sorry).

(In reply to comment #12)
> Shouldn't we use an autoconf test or something other than a tangle of compiler
> and OS ifdefs?

I agree, but my patch follows the original patch.

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to comment #8)
> looks like we use gcc on WinMo, but there we have it:

FWIW, we use a horrible little wrapper that looks like GCC to call MSVC for WinCE builds.

Comment 17

[Jason Orendorff [:jorendorff]]

Filed bug 510895 to tidy up the ifdefs.

(In reply to comment #13)
> changeset b211fcf538a9 breaks build using MSVC 7.1.

Roy, if you'd like to contribute a patch for bug 510895, it would be welcome, but even if not, please leave a comment there with details about the breakage, including complete error messages. Thanks.

Comment 18

[Roy Tam]

(In reply to comment #17)
> Filed bug 510895 to tidy up the ifdefs.
> 
> (In reply to comment #13)
> > changeset b211fcf538a9 breaks build using MSVC 7.1.
> 
> Roy, if you'd like to contribute a patch for bug 510895, it would be welcome,
> but even if not, please leave a comment there with details about the breakage,
> including complete error messages. Thanks.

There's no _getc_nolock in MSVC 7.1.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 394646 [details] [diff] [review]
mingw fix

Jacek, does the patch in bug 510895 work for you?

Comment 20

[Jacek Caban]

Yes, it works great. Thank you.