Bug 76285 – Refactor identifier resolution in BytecodeGenerator

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy Wingo]

The patch to be attached will refactor identifier resolution, as suggested in bug 75708 comment 2.

[Andy Wingo]

Created attachment 122450 [details]
Patch

[Andy Wingo]

The attached patch is fairly large, but it shouldn't change anything semantically.  It passes run-javascriptcore-tests with and without debug.

What do you think, Geoffrey?

[Geoffrey Garen]

General direction looks good to me. One note for now, before a full review: you'll want to run run-webkit-tests for correctness testing, and bencher for performance testing, as well.

[Andy Wingo]

Created attachment 122631 [details]
Patch

[Andy Wingo]

Comment on attachment 122631 [details]
Patch

This updated patch fixes a bug that run-webkit-tests caught, in which resolve() could return an invalid "type".

The test results seem OK:


27935 tests ran as expected, 4 didn't:              


Unexpected flakiness: text diff mismatch (3)
  editing/pasteboard/pasting-empty-html-falls-back-to-text.html = TEXT PASS
  fast/forms/basic-buttons.html = TEXT PASS
  media/video-poster-blocked-by-willsendrequest.html = TEXT PASS


Regressions: Unexpected image mismatch : (1)
  svg/foreignObject/repaint-rect-coordinates.html = IMAGE


I'm assuming those problems have nothing to do with this patch.

[Andy Wingo]

Benchmark report for SunSpider, V8, and Kraken on \c.

VMs tested:
"trunk" at /home/wingo/src/WebKit/WebKitBuild/Release-trunk/Programs/jsc
"refactor" at /home/wingo/src/WebKit/WebKitBuild/Release/Programs/jsc

Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime()
function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in
milliseconds.

                                              trunk                  refactor                                    
SunSpider:
   3d-cube                                5.4096+-0.2538          5.1875+-0.2001         might be 1.0428x faster
   3d-morph                               7.6254+-0.3902    ?     7.6515+-0.2127       ?
   3d-raytrace                            8.1438+-0.6973          7.5884+-0.3058         might be 1.0732x faster
   access-binary-trees                    1.7641+-0.1350    ?     1.7677+-0.1208       ?
   access-fannkuch                       10.9894+-0.1674         10.8687+-0.1637         might be 1.0111x faster
   access-nbody                           3.0872+-0.2183    ?     3.1068+-0.4445       ?
   access-nsieve                          2.7876+-0.2191    ?     3.0385+-0.2362       ? might be 1.0900x slower
   bitops-3bit-bits-in-byte               1.5567+-0.2422          1.4197+-0.0994         might be 1.0966x faster
   bitops-bits-in-byte                    2.4268+-0.1596          2.3921+-0.1263         might be 1.0145x faster
   bitops-bitwise-and                     6.4219+-0.2260          6.4202+-0.3489       
   bitops-nsieve-bits                     5.7165+-0.4385          5.4790+-0.1839         might be 1.0433x faster
   controlflow-recursive                  2.0563+-0.1433    ?     2.2568+-0.3584       ? might be 1.0975x slower
   crypto-aes                             6.5085+-0.3982    ?     6.5795+-0.3358       ? might be 1.0109x slower
   crypto-md5                             2.9482+-0.6833          2.3396+-0.1538         might be 1.2601x faster
   crypto-sha1                            2.2550+-0.3891    ?     2.4811+-0.3457       ? might be 1.1002x slower
   date-format-tofte                      9.9916+-0.4122    ?    10.0765+-0.3077       ?
   date-format-xparb                      8.6556+-0.2092    ?     8.7391+-0.3008       ?
   math-cordic                            6.0482+-0.1594          6.0479+-0.1019       
   math-partial-sums                     13.7263+-0.2892    ?    13.9744+-0.3798       ? might be 1.0181x slower
   math-spectral-norm                     2.5806+-0.2523          2.5211+-0.1493         might be 1.0236x faster
   regexp-dna                             7.8237+-0.2436    ?     7.8756+-0.3009       ?
   string-base64                          5.1418+-0.3378    ?     5.3956+-0.2006       ? might be 1.0493x slower
   string-fasta                           7.1104+-0.2327    ?     7.3781+-0.3737       ? might be 1.0376x slower
   string-tagcloud                       10.8687+-0.4135         10.8683+-0.3032       
   string-unpack-code                    17.9470+-0.2351         17.8377+-0.1830       
   string-validate-input                  6.2236+-0.4266          6.1110+-0.4341         might be 1.0184x faster

   <arithmetic> *                         6.3775+-0.1268          6.3616+-0.0999         might be 1.0025x faster
   <geometric>                            5.1820+-0.1233          5.1687+-0.1096         might be 1.0026x faster
   <harmonic>                             4.1174+-0.1305          4.1101+-0.1179         might be 1.0018x faster

                                              trunk                  refactor                                    
V8:
   crypto                                66.6464+-0.5153    ?    67.0794+-0.8050       ?
   deltablue                            146.7019+-1.9685        146.0889+-0.8053       
   earley-boyer                          81.9158+-1.1611         81.6925+-0.8074       
   raytrace                              47.3773+-0.6577    ?    47.5151+-0.6876       ?
   regexp                                82.2627+-0.4184    ?    82.3784+-0.3202       ?
   richards                             113.0822+-0.9629        112.1683+-0.4324       
   splay                                 95.6887+-1.0875    ?    95.9071+-0.5643       ?

   <arithmetic>                          90.5250+-0.5047         90.4042+-0.2401         might be 1.0013x faster
   <geometric> *                         85.6268+-0.4159         85.6057+-0.2542         might be 1.0002x faster
   <harmonic>                            80.8258+-0.3834    ?    80.8900+-0.3075       ? might be 1.0008x slower

                                              trunk                  refactor                                    
Kraken:
   ai-astar                             446.0029+-2.1009        445.8995+-1.2161       
   audio-beat-detection                 174.9487+-0.4722    ?   177.2184+-1.9397       ? might be 1.0130x slower
   audio-dft                            344.4122+-3.4931    ?   350.0982+-4.0717       ? might be 1.0165x slower
   audio-fft                            114.6621+-0.7328    ?   115.1699+-0.9732       ?
   audio-oscillator                     231.6501+-4.1325        231.1128+-3.4095       
   imaging-darkroom                     308.3783+-7.1186    ?   308.7481+-6.3187       ?
   imaging-desaturate                   204.0813+-1.4658        203.4175+-0.6241       
   imaging-gaussian-blur                463.3229+-3.0306        463.0983+-2.4223       
   json-parse-financial                  51.5524+-0.6663         51.1047+-0.1872       
   json-stringify-tinderbox              64.8833+-0.6971         64.2974+-0.5993       
   stanford-crypto-aes                   92.3050+-0.6536         91.8039+-0.4008       
   stanford-crypto-ccm                   92.4999+-0.8191    ?    92.6817+-0.7803       ?
   stanford-crypto-pbkdf2               171.7288+-1.2792    ?   171.8636+-1.0281       ?
   stanford-crypto-sha256-iterative      71.7853+-0.1899         71.7697+-0.5174       

   <arithmetic> *                       202.3009+-1.0974    ?   202.7346+-0.8915       ? might be 1.0021x slower
   <geometric>                          160.1750+-0.7016    ?   160.2683+-0.6107       ? might be 1.0006x slower
   <harmonic>                           126.7766+-0.4536        126.5421+-0.4134         might be 1.0019x faster

                                              trunk                  refactor                                    
All benchmarks:
   <arithmetic>                          77.2703+-0.3683    ?    77.3727+-0.2905       ? might be 1.0013x slower
   <geometric>                           21.8632+-0.3058         21.8357+-0.2655         might be 1.0013x faster
   <harmonic>                             7.2173+-0.2221          7.2049+-0.2004         might be 1.0017x faster

                                              trunk                  refactor                                    
Geomean of preferred means:
   <scaled-result>                       47.9782+-0.3838         47.9703+-0.2887         might be 1.0002x faster

[Andy Wingo]

In summary, performance-neutral, I think.  I'm ready for a commit! ;-)

[Andy Wingo]

Kind ping to Geoffrey :-)

[Andy Wingo]

Unfortunately, I think that my build in Release-trunk was dynamically linking to the build in Release, so my previous results are bogus.  For some reason bencher is not running at all for me on trunk, with clean source and build trees, so here are some (pretty poor) sunspider and v8 results for this patch:


TEST                   COMPARISON            FROM                 TO             DETAILS

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

** TOTAL **:           *1.076x as slow*  157.8ms +/- 0.9%   169.7ms +/- 1.1%     significant

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

  3d:                  ??                 23.2ms +/- 3.0%    23.9ms +/- 4.8%     not conclusive: might be *1.028x as slow*
    cube:              ??                  7.1ms +/- 9.7%     7.5ms +/- 15.1%     not conclusive: might be *1.066x as slow*
    morph:             ??                  8.2ms +/- 2.7%     8.5ms +/- 3.7%     not conclusive: might be *1.035x as slow*
    raytrace:          -                   7.9ms +/- 5.6%     7.8ms +/- 4.7% 

  access:              *1.33x as slow*    14.0ms +/- 1.7%    18.7ms +/- 1.2%     significant
    binary-trees:      -                   2.2ms +/- 7.3%     2.1ms +/- 9.4% 
    fannkuch:          *1.81x as slow*     6.1ms +/- 2.3%    11.0ms +/- 1.9%     significant
    nbody:             1.082x as fast      3.2ms +/- 1.8%     2.9ms +/- 4.0%     significant
    nsieve:            ??                  2.6ms +/- 7.7%     2.6ms +/- 5.5%     not conclusive: might be *1.007x as slow*

  bitops:              *1.28x as slow*    11.8ms +/- 2.1%    15.0ms +/- 1.7%     significant
    3bit-bits-in-byte: ??                  1.3ms +/- 4.8%     1.4ms +/- 8.1%     not conclusive: might be *1.068x as slow*
    bits-in-byte:      ??                  2.3ms +/- 3.6%     2.3ms +/- 4.4%     not conclusive: might be *1.023x as slow*
    bitwise-and:       *2.04x as slow*     3.0ms +/- 1.5%     6.1ms +/- 2.0%     significant
    nsieve-bits:       -                   5.2ms +/- 3.5%     5.2ms +/- 2.9% 

  controlflow:         ??                  1.9ms +/- 3.5%     2.0ms +/- 5.0%     not conclusive: might be *1.054x as slow*
    recursive:         ??                  1.9ms +/- 3.5%     2.0ms +/- 5.0%     not conclusive: might be *1.054x as slow*

  crypto:              1.160x as fast     12.6ms +/- 3.4%    10.8ms +/- 4.6%     significant
    aes:               1.22x as fast       8.0ms +/- 5.0%     6.5ms +/- 4.4%     significant
    md5:               1.110x as fast      2.5ms +/- 4.5%     2.3ms +/- 9.4%     significant
    sha1:              -                   2.1ms +/- 2.8%     2.0ms +/- 8.9% 

  date:                *1.069x as slow*   19.6ms +/- 1.7%    20.9ms +/- 3.5%     significant
    format-tofte:      -                  10.5ms +/- 2.3%    10.5ms +/- 2.6% 
    format-xparb:      *1.153x as slow*    9.0ms +/- 2.6%    10.4ms +/- 5.9%     significant

  math:                *1.28x as slow*    17.3ms +/- 1.4%    22.1ms +/- 0.7%     significant
    cordic:            -                   6.3ms +/- 1.9%     6.3ms +/- 4.1% 
    partial-sums:      *1.57x as slow*     8.6ms +/- 1.8%    13.5ms +/- 1.4%     significant
    spectral-norm:     ??                  2.3ms +/- 3.9%     2.3ms +/- 2.9%     not conclusive: might be *1.006x as slow*

  regexp:              -                   8.4ms +/- 2.6%     8.3ms +/- 2.5% 
    dna:               -                   8.4ms +/- 2.6%     8.3ms +/- 2.5% 

  string:              1.025x as fast     49.1ms +/- 0.8%    47.9ms +/- 1.4%     significant
    base64:            *1.116x as slow*    4.5ms +/- 4.4%     5.0ms +/- 3.5%     significant
    fasta:             1.039x as fast      7.3ms +/- 3.0%     7.0ms +/- 2.2%     significant
    tagcloud:          1.075x as fast     12.0ms +/- 1.4%    11.2ms +/- 2.4%     significant
    unpack-code:       1.048x as fast     19.8ms +/- 1.8%    18.9ms +/- 2.2%     significant
    validate-input:    *1.056x as slow*    5.4ms +/- 2.0%     5.8ms +/- 2.5%     significant




TEST              COMPARISON            FROM                 TO             DETAILS

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

** TOTAL **:      *1.013x as slow*  700.8ms +/- 0.5%   710.0ms +/- 0.8%     significant

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

  v8:             *1.013x as slow*  700.8ms +/- 0.5%   710.0ms +/- 0.8%     significant
    crypto:       1.016x as fast     72.7ms +/- 0.8%    71.5ms +/- 1.5%     significant
    deltablue:    *1.074x as slow*  153.7ms +/- 0.6%   165.0ms +/- 1.3%     significant
    earley-boyer: -                  82.8ms +/- 1.9%    82.3ms +/- 1.6% 
    raytrace:     -                  48.6ms +/- 1.3%    48.2ms +/- 1.2% 
    regexp:       ??                 87.4ms +/- 0.8%    87.9ms +/- 1.2%     not conclusive: might be *1.007x as slow*
    richards:     -                 113.5ms +/- 0.3%   113.1ms +/- 0.8% 
    splay:        -                 142.1ms +/- 1.0%   141.9ms +/- 0.8%

[Andy Wingo]

The slowdown on fannkuch makes me think that somehow we are not emitting the optimal code.  I'll look into it.

[Andy Wingo]

Created attachment 122912 [details]
Patch

[Andy Wingo]

Comment on attachment 122912 [details]
Patch

There were some cases in which resolve_global_dynamic was being emitted instead of resolve_global.

With this patch, performance is indeed neutral.

Sunspider results:


TEST                   COMPARISON            FROM                 TO             DETAILS

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

** TOTAL **:           -                 155.0ms +/- 0.8%   154.9ms +/- 0.7% 

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

  3d:                  -                  24.5ms +/- 4.0%    23.9ms +/- 3.8% 
    cube:              -                   8.3ms +/- 11.9%     7.5ms +/- 12.4% 
    morph:             1.044x as fast      8.6ms +/- 3.0%     8.3ms +/- 2.0%     significant
    raytrace:          *1.076x as slow*    7.5ms +/- 3.7%     8.1ms +/- 2.7%     significant

  access:              ??                 13.4ms +/- 0.9%    13.7ms +/- 2.0%     not conclusive: might be *1.019x as slow*
    binary-trees:      *1.040x as slow*    1.9ms +/- 1.0%     2.0ms +/- 3.8%     significant
    fannkuch:          ??                  5.8ms +/- 1.8%     5.9ms +/- 2.1%     not conclusive: might be *1.008x as slow*
    nbody:             ??                  3.1ms +/- 0.7%     3.2ms +/- 5.3%     not conclusive: might be *1.041x as slow*
    nsieve:            ??                  2.6ms +/- 3.7%     2.6ms +/- 1.9%     not conclusive: might be *1.005x as slow*

  bitops:              -                  11.7ms +/- 1.2%    11.6ms +/- 1.1% 
    3bit-bits-in-byte: -                   1.3ms +/- 5.2%     1.3ms +/- 1.5% 
    bits-in-byte:      -                   2.2ms +/- 3.0%     2.2ms +/- 1.3% 
    bitwise-and:       ??                  3.0ms +/- 0.8%     3.0ms +/- 1.3%     not conclusive: might be *1.006x as slow*
    nsieve-bits:       -                   5.2ms +/- 1.6%     5.1ms +/- 2.3% 

  controlflow:         -                   1.9ms +/- 2.3%     1.9ms +/- 0.8% 
    recursive:         -                   1.9ms +/- 2.3%     1.9ms +/- 0.8% 

  crypto:              1.029x as fast     12.2ms +/- 2.8%    11.9ms +/- 0.5%     significant
    aes:               -                   7.6ms +/- 3.4%     7.4ms +/- 0.9% 
    md5:               -                   2.6ms +/- 7.5%     2.4ms +/- 1.6% 
    sha1:              -                   2.1ms +/- 5.3%     2.1ms +/- 2.2% 

  date:                ??                 19.6ms +/- 2.0%    19.9ms +/- 0.7%     not conclusive: might be *1.016x as slow*
    format-tofte:      ??                 10.5ms +/- 2.1%    10.5ms +/- 0.7%     not conclusive: might be *1.006x as slow*
    format-xparb:      ??                  9.1ms +/- 3.1%     9.4ms +/- 1.7%     not conclusive: might be *1.028x as slow*

  math:                -                  17.1ms +/- 1.4%    17.0ms +/- 1.2% 
    cordic:            ??                  6.2ms +/- 1.7%     6.3ms +/- 1.5%     not conclusive: might be *1.012x as slow*
    partial-sums:      -                   8.5ms +/- 1.5%     8.5ms +/- 1.3% 
    spectral-norm:     -                   2.3ms +/- 4.1%     2.3ms +/- 2.1% 

  regexp:              ??                  8.2ms +/- 1.7%     8.3ms +/- 2.2%     not conclusive: might be *1.008x as slow*
    dna:               ??                  8.2ms +/- 1.7%     8.3ms +/- 2.2%     not conclusive: might be *1.008x as slow*

  string:              *1.007x as slow*   46.4ms +/- 0.5%    46.7ms +/- 0.4%     significant
    base64:            1.023x as fast      4.4ms +/- 1.8%     4.3ms +/- 1.2%     significant
    fasta:             ??                  6.9ms +/- 1.2%     7.0ms +/- 1.4%     not conclusive: might be *1.013x as slow*
    tagcloud:          ??                 11.0ms +/- 1.2%    11.0ms +/- 1.2%     not conclusive: might be *1.002x as slow*
    unpack-code:       ??                 18.6ms +/- 0.7%    18.7ms +/- 0.6%     not conclusive: might be *1.002x as slow*
    validate-input:    *1.051x as slow*    5.4ms +/- 2.5%     5.7ms +/- 1.3%     significant


and v8-v6:


TEST              COMPARISON            FROM                 TO             DETAILS

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

** TOTAL **:      -                 692.3ms +/- 0.7%   692.7ms +/- 0.2% 

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

  v8:             -                 692.3ms +/- 0.7%   692.7ms +/- 0.2% 
    crypto:       -                  72.2ms +/- 1.1%    72.1ms +/- 0.9% 
    deltablue:    ??                152.0ms +/- 1.2%   153.8ms +/- 0.6%     not conclusive: might be *1.011x as slow*
    earley-boyer: -                  80.5ms +/- 1.1%    80.3ms +/- 0.6% 
    raytrace:     1.018x as fast     48.1ms +/- 1.2%    47.2ms +/- 1.0%     significant
    regexp:       -                  86.5ms +/- 1.1%    86.4ms +/- 0.7% 
    richards:     -                 112.7ms +/- 1.0%   112.6ms +/- 0.4% 
    splay:        -                 140.4ms +/- 1.2%   140.3ms +/- 0.3%

[Geoffrey Garen]

Comment on attachment 122912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122912&action=review

> Source/JavaScriptCore/ChangeLog:1
> [phone]  Andy Wingo  <[e-mail]>

Nice ChangeLog -- I like how you commented about individual changes, and the "why" of them.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:507
> +RegisterID* BytecodeGenerator::registerFor(const ResolveResult& location)

When you have a trivial declaration like this function argument, I usually prefer the naming style "const ClassName& className". In this case: "const ResolveResult& resolveResult". That way, the one thing doesn't have two names. I think that would be better here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:512
> +    return createLazyRegisterIfNecessary(&registerFor(location.index));

This behavior (creating a lazy register if necessary) should move into BytecodeGenerator::resolve(), since it already has the responsibility of creating the arguments register lazily. It's nice to have all lazy register allocation in one place. (Eventually, we might even give resolve() the responsibility for emitting opcodes like op_resolve and op_get_scoped_var.)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1137
> +    ResolveResult ret = { 0, 0, 0, 0 };

All zeroes should be the default constructor for ResolveResult, so it doesn't have to be specified explicitly.

Notice how here you called the ResolveResult "ret", while in other places you called it "location". It would be more uniform to standardize on calling it "resolveResult" everywhere.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1141
> +        ret.type = ResolveResult::ImmutableRegister;
> +        ret.index = m_thisRegister.index();

This data should be private in ResolveResult, with setter functions for the different kinds of resolves. That way, a programmer can't make the error of setting a type that is internally inconsistent with the other information in the ResolveResult.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1153
> +            if (entry.isReadOnly())
> +                ret.type |= ResolveResult::ImmutableFlag;

This would read better if the ResolveResult flag were named "ReadOnlyFlag", to match the "ReadOnly" terminology in SymbolTable.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1429
> +RegisterID* BytecodeGenerator::emitGetStaticVar(RegisterID* dst, const ResolveResult& location)

I like how this function turned out.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1438
> +        if (dst == ignoredResult())
> +            return 0;

This would read better, and be slightly more efficient, if you did the early return check before the call to registerFor().

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:151
> +    ResolveResult location = generator.resolve(m_ident);
> +    if (RegisterID* local = generator.registerFor(location)) {

I think you could simplify even further by merging ResolveResult and regsiterFor(), like so:

if (RegisterID* local = resolveResult.local()) { // NULL if not a local variable
    // Code for local variable goes here
}

In a way, the existing BytecodeGenerator::registerFor() is a weak primordial version of the behavior we want out of BytecodeGenerator::resolve() + ResolveResult.

[Geoffrey Garen]

Comment on attachment 122912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122912&action=review

> Source/JavaScriptCore/ChangeLog:1
> [phone]  Andy Wingo  <[e-mail]>

Nice ChangeLog -- I like how you commented about individual changes, and the "why" of them.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:507
> +RegisterID* BytecodeGenerator::registerFor(const ResolveResult& location)

When you have a trivial declaration like this function argument, I usually prefer the naming style "const ClassName& className". In this case: "const ResolveResult& resolveResult". That way, the one thing doesn't have two names. I think that would be better here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:512
> +    return createLazyRegisterIfNecessary(&registerFor(location.index));

This behavior (creating a lazy register if necessary) should move into BytecodeGenerator::resolve(), since it already has the responsibility of creating the arguments register lazily. It's nice to have all lazy register allocation in one place. (Eventually, we might even give resolve() the responsibility for emitting opcodes like op_resolve and op_get_scoped_var.)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1137
> +    ResolveResult ret = { 0, 0, 0, 0 };

All zeroes should be the default constructor for ResolveResult, so it doesn't have to be specified explicitly.

Notice how here you called the ResolveResult "ret", while in other places you called it "location". It would be more uniform to standardize on calling it "resolveResult" everywhere.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1141
> +        ret.type = ResolveResult::ImmutableRegister;
> +        ret.index = m_thisRegister.index();

This data should be private in ResolveResult, with setter functions for the different kinds of resolves. That way, a programmer can't make the error of setting a type that is internally inconsistent with the other information in the ResolveResult.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1153
> +            if (entry.isReadOnly())
> +                ret.type |= ResolveResult::ImmutableFlag;

This would read better if the ResolveResult flag were named "ReadOnlyFlag", to match the "ReadOnly" terminology in SymbolTable.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1429
> +RegisterID* BytecodeGenerator::emitGetStaticVar(RegisterID* dst, const ResolveResult& location)

I like how this function turned out.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1438
> +        if (dst == ignoredResult())
> +            return 0;

This would read better, and be slightly more efficient, if you did the early return check before the call to registerFor().

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:151
> +    ResolveResult location = generator.resolve(m_ident);
> +    if (RegisterID* local = generator.registerFor(location)) {

I think you could simplify even further by merging ResolveResult and regsiterFor(), like so:

if (RegisterID* local = resolveResult.local()) { // NULL if not a local variable
    // Code for local variable goes here
}

In a way, the existing BytecodeGenerator::registerFor() is a weak primordial version of the behavior we want out of BytecodeGenerator::resolve() + ResolveResult.

[Geoffrey Garen]

Comment on attachment 122912 [details]
Patch

I like how this patch turned out, but I think it will be even better with a round of cleanup before landing.

[Andy Wingo]

Thanks for the review; I'll post an updated patch on Monday.  Have a lovely weekend.

[Andy Wingo]

Created attachment 123574 [details]
Patch

[Andy Wingo]

(In reply to comment #14)
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:512
> > +    return createLazyRegisterIfNecessary(&registerFor(location.index));
> 
> This behavior (creating a lazy register if necessary) should move into BytecodeGenerator::resolve(), since it already has the responsibility of creating the arguments register lazily. It's nice to have all lazy register allocation in one place. (Eventually, we might even give resolve() the responsibility for emitting opcodes like op_resolve and op_get_scoped_var.)

I didn't do this in my new patch, because I misunderstood what you were saying.  I'll fix this tomorrow morning if this patch doesn't go in tonight.

> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1137
> > +    ResolveResult ret = { 0, 0, 0, 0 };
> 
> All zeroes should be the default constructor for ResolveResult, so it doesn't have to be specified explicitly.

I wasn't sure what the right style here was; please have a look.  (I'm an old C and Scheme hand, and my C++ could use some improvement.)

> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1141
> > +        ret.type = ResolveResult::ImmutableRegister;
> > +        ret.index = m_thisRegister.index();
> 
> This data should be private in ResolveResult, with setter functions for the different kinds of resolves. That way, a programmer can't make the error of setting a type that is internally inconsistent with the other information in the ResolveResult.

It should only be this one function initializing these fields, though I grant that it would be good to make that assumption manifest; a case for a friend class?  Or should I add the panoply of getters and setters?  (Sorry for the naive questions, I still have bad C++ style instincts :)

> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:151
> > +    ResolveResult location = generator.resolve(m_ident);
> > +    if (RegisterID* local = generator.registerFor(location)) {
> 
> I think you could simplify even further by merging ResolveResult and regsiterFor(), like so:
> 
> if (RegisterID* local = resolveResult.local()) { // NULL if not a local variable
>     // Code for local variable goes here
> }
> 
> In a way, the existing BytecodeGenerator::registerFor() is a weak primordial version of the behavior we want out of BytecodeGenerator::resolve() + ResolveResult.

You want resolveResult() to hold on to the generator, or have it have a RegisterID* pointer in it?  I'm fine with either; the current code reflects my lack of understanding of the memory ownership of registerid values.

Thanks for the comments,

Andy

[Geoffrey Garen]

> > This behavior (creating a lazy register if necessary) should move into BytecodeGenerator::resolve()
> 
> I didn't do this in my new patch, because I misunderstood what you were saying.  I'll fix this tomorrow morning if this patch doesn't go in tonight.

Okiedokie.

> > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1137
> > > +    ResolveResult ret = { 0, 0, 0, 0 };
> > 
> > All zeroes should be the default constructor for ResolveResult, so it doesn't have to be specified explicitly.
> 
> I wasn't sure what the right style here was; please have a look.  (I'm an old C and Scheme hand, and my C++ could use some improvement.)

+    ResolveResult resolveResult = ResolveResult();

You can write this more succinctly as:

ResolveResult resolveResult;

You need to add a constructor declaration and definition to the ResolveResult class in order for this to do the right thing. Otherwise, you have uninitialized memory. Here's an example definition:

inline ResolveResult::ResolveResult()
    : m_type(0)
    , m_index(0)
    , m_depth(0)
    , m_globalObject(0)
{
}

> > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1141
> > > +        ret.type = ResolveResult::ImmutableRegister;
> > > +        ret.index = m_thisRegister.index();
> > 
> > This data should be private in ResolveResult, with setter functions for the different kinds of resolves. That way, a programmer can't make the error of setting a type that is internally inconsistent with the other information in the ResolveResult.
> 
> It should only be this one function initializing these fields, though I grant that it would be good to make that assumption manifest; a case for a friend class?  Or should I add the panoply of getters and setters?  (Sorry for the naive questions, I still have bad C++ style instincts :)

I would go either with the panoply of (public) getters and setters with (private) data, or with dedicated "create" functions and private data, making ResolveResult a class instead of a struct. These will help make explicit invariants like "If and only if you have a global resolve, you must set type to ResolveResult::Global and globalObject to a JSGlobalObject*".

Not better:

resolveResult.setType(ResolveResult::Lexical);
resolveResult.setDepth(depth);
resolveResult.setIndex(entry.getIndex());
return resolveResult;

Better:

return ResolveResult::lexicalResolve(depth, entry.getIndex()); // Now, the client always knows what data to provide, and doesn't need to know how that data will be encoded.

> > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:151
> > > +    ResolveResult location = generator.resolve(m_ident);
> > > +    if (RegisterID* local = generator.registerFor(location)) {
> > 
> > I think you could simplify even further by merging ResolveResult and regsiterFor(), like so:
> > 
> > if (RegisterID* local = resolveResult.local()) { // NULL if not a local variable
> >     // Code for local variable goes here
> > }
> > 
> > In a way, the existing BytecodeGenerator::registerFor() is a weak primordial version of the behavior we want out of BytecodeGenerator::resolve() + ResolveResult.
> 
> You want resolveResult() to hold on to the generator, or have it have a RegisterID* pointer in it?

RegisterID*.

Temporary RegisterIDs require reference counting. RegisterIDs for local variables are "immortal", and you can refer to them with raw pointers.

[Andy Wingo]

Created attachment 123742 [details]
Patch

[Andy Wingo]

I believe the newly attached patch has addressed all of your comments, Geoffrey.  Thanks again for the review.

[Andy Wingo]

I don't know how to interpret this, but here's the results of run-webkit-tests --gtk:


Unexpected flakiness: text diff mismatch (6)
  editing/pasteboard/pasting-empty-html-falls-back-to-text.html = TEXT PASS
  fast/js/navigator-language.html = TEXT PASS
  http/tests/appcache/abort-cache-ondownloading-resource-404.html = TEXT PASS
  media/video-poster-blocked-by-willsendrequest.html = TEXT PASS
  sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A4_T2.html = TEXT PASS
  sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A4_T3.html = TEXT PASS


Regressions: Unexpected text diff mismatch : (18)
  css3/calc/getComputedStyle-margin-percentage.html = TEXT
  editing/execCommand/insertImage.html = TEXT
  editing/pasteboard/drag-image-to-contenteditable-in-iframe.html = TEXT
  fast/block/float/015.html = TEXT
  fast/borders/rtl-border-05.html = TEXT
  fast/dom/34176.html = TEXT
  fast/dom/inner-text.html = TEXT
  fast/encoding/utf-16-big-endian.html = TEXT
  fast/encoding/utf-16-little-endian.html = TEXT
  fast/events/clear-drag-state.html = TEXT
  fast/events/clear-edit-drag-state.html = TEXT
  fast/hidpi/broken-image-icon-hidpi.html = TEXT
  fast/js/array-functions-non-arrays.html = TEXT
  http/tests/media/video-buffering-repaints-controls.html = TEXT
  sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A3_T1.html = TEXT
  tables/mozilla/bugs/bug2962.html = TEXT
  tables/mozilla/bugs/bug56201.html = TEXT
  tables/mozilla/collapsing_borders/bug41262-3.html = TEXT


Current as of r105717.

[Andy Wingo]

A friendly ping to Geoffrey for review.

Cheers,

Andy

[Andy Wingo]

Geoffrey, would you mind reviewing this patch?  Thanks!

Andy

[Geoffrey Garen]

Comment on attachment 123742 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 123742 [details]
Patch

Clearing flags on attachment: 123742

Committed r106478: <http://trac.webkit.org/changeset/106478>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This has caused bug 87158.