Bug 136056 - REGRESSION(r172401): for-in optimization no longer works at all
Summary: REGRESSION(r172401): for-in optimization no longer works at all
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 136058 136124
Blocks:
  Show dependency treegraph
 
Reported: 2014-08-18 19:31 PDT by Filip Pizlo
Modified: 2014-08-21 01:05 PDT (History)
12 users (show)

See Also:


Attachments
the wrong approach (4.09 KB, patch)
2014-08-18 19:32 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (8.73 KB, patch)
2014-08-18 19:37 PDT, Filip Pizlo
mhahnenb: review+
Details | Formatted Diff | Diff
the patch (13.09 KB, patch)
2014-08-19 17:10 PDT, Filip Pizlo
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (498.85 KB, application/zip)
2014-08-19 18:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (501.15 KB, application/zip)
2014-08-19 19:37 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-08-18 19:31:09 PDT
That patch appears to be flat out wrong, though its test is mildly useful.

It's actually perfectly safe to issue a get_direct_pname for the wrong base because of the structure check.
Comment 1 Filip Pizlo 2014-08-18 19:32:26 PDT
Created attachment 236794 [details]
the wrong approach

I initially tried to do a direct fix to the alleged bug by having dynamic checks to see if the base matched.  Then I realized that this is not needed.  But, I'm saving this approach here just in case.
Comment 2 Filip Pizlo 2014-08-18 19:37:32 PDT
Created attachment 236795 [details]
the patch
Comment 3 Mark Hahnenberg 2014-08-18 19:40:31 PDT
Comment on attachment 236795 [details]
the patch

r=me
Comment 4 Mark Lam 2014-08-18 21:44:47 PDT
This patch was landed in r172741: <http://trac.webkit.org/r172741>.

And then rolled out in r172742: <http://trac.webkit.org/r172742> due to https://bugs.webkit.org/show_bug.cgi?id=136058.
Comment 5 Geoffrey Garen 2014-08-19 13:08:50 PDT
> It's actually perfectly safe to issue a get_direct_pname for the wrong base because of the structure check.

It's true that it's safe, but it's probably not desirable.
Comment 6 Filip Pizlo 2014-08-19 17:08:06 PDT
(In reply to comment #5)
> > It's actually perfectly safe to issue a get_direct_pname for the wrong base because of the structure check.
> 
> It's true that it's safe, but it's probably not desirable.

It is desirable.

For starters, it's completely sound.  Given some structure S and some string foo, we know that there exists an offset such that any access o[foo] where o has structure S can be performed by just using that offset.

Also, it is very much desirable because there does not exist any other alternative for how to make for-in fast.  There is no point in trying to statically prove whether the base matches.  In no realistic situation will we have an efficient mechanism for detecting if the base matches, other than doing a dynamic base check, which would obviously be worse than the structure check that we are doing anyway.  Consider some examples:

FOR-IN WITH LOCAL VARIABLE BASE

for (var s in o) {
    o[s]
}

Except for very trivial cases, we cannot be sure that the body of the for loop has a reassignment of o, or that this reassignment actually interferes with the optimization:

for (var s in o) {
    o[s]
    o = foo;
}

In SSA form - which our bytecode doesn't have - we could implement a cheap check for this.  In our bytecode's 3AC form we would have to do some additional analysis to ensure that no such statement is present.  This additional analysis would have no positive effect on the system because it would not increase soundness or performance.  It would cost additional time during bytecode parsing, but it would achieve nothing, since the structure check was already adequate.

FOR-IN WITH NON-LOCAL-VARIABLE BASE

for (var s in o.f.f.f) {
    o.f.f.f[s]
}

You have two options when bytecompiling o.f.f.f[s]:

- Do what we have always done, which is emit a get_direct_pname.  This is obviously the right choice for this program.

- Emit a raw get_by_val.  This is never better.  get_by_val with a string base is horribly expensive; it's guaranteed to be an out-of-line call.  get_direct_pname will make that out-of-line call, but only after doing a structure check.  That structure check is essentially free if you consider the relative expense of the out-of-line get_by_val that we would do anyway.

Hence, get_direct_pname is always the better choice, if you know that the subscript of the access is the iterator variable of a for-in loop and the other conditions for get_direct_pname hold.

Finally, in the higher tiers, when we have additional profiling to tell us what is going on, it is always beneficial to have a get_by_val with a for-in iterator subscript to be able to refer to the for-in loop's index and enumerator object.  That's all that get_direct_pname means: it's semantically a get_by_val with a hint that says "by the way, I know statically that the subscript string emerged from an enumerator during some enumeration index".  The compiler can still turn it into a direct out-of-line get_by_val call if it believes that the base object will never have the right structure.
Comment 7 Filip Pizlo 2014-08-19 17:10:38 PDT
Created attachment 236836 [details]
the patch
Comment 8 Geoffrey Garen 2014-08-19 17:13:16 PDT
Comment on attachment 236836 [details]
the patch

r=me
Comment 9 Build Bot 2014-08-19 18:31:56 PDT
Comment on attachment 236836 [details]
the patch

Attachment 236836 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5342671258255360

New failing tests:
http/tests/security/cross-frame-access-enumeration.html
Comment 10 Build Bot 2014-08-19 18:31:59 PDT
Created attachment 236844 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 11 Filip Pizlo 2014-08-19 19:32:03 PDT
(In reply to comment #9)
> (From update of attachment 236836 [details])
> Attachment 236836 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/5342671258255360
> 
> New failing tests:
> http/tests/security/cross-frame-access-enumeration.html

Looks like this just needs a rebase because the number of calls into the DOM has changed and so the number of console messages about security stuff has now changed.
Comment 12 Build Bot 2014-08-19 19:37:51 PDT
Comment on attachment 236836 [details]
the patch

Attachment 236836 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4688001472921600

New failing tests:
http/tests/security/cross-frame-access-enumeration.html
Comment 13 Build Bot 2014-08-19 19:37:57 PDT
Created attachment 236848 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Filip Pizlo 2014-08-19 19:39:34 PDT
Landed in http://trac.webkit.org/changeset/172794
Comment 16 Filip Pizlo 2014-08-20 16:20:03 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Landed in http://trac.webkit.org/changeset/172794
> 
> It broke zillion tests on the Apple Mac 32 bit bot:http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/3680/steps/webkit-32bit-jsc-test/logs/stdio

And by "zillion", you of course mean "two newly added tests in this revision".
Comment 17 Csaba Osztrogonác 2014-08-20 23:58:31 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Landed in http://trac.webkit.org/changeset/172794
> > 
> > It broke zillion tests on the Apple Mac 32 bit bot:http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/3680/steps/webkit-32bit-jsc-test/logs/stdio
> 
> And by "zillion", you of course mean "two newly added tests in this revision".

:)

I checked only the number: "Rsults for JSC stress tests: 22 failures found.",
but you are true, zillion means _now_ the only two new tests in every config.
But it doesn't mean if they aren't fail and you aren't responsible to fix them.
Comment 18 Csaba Osztrogonác 2014-08-21 01:05:51 PDT
The new bug report to the track the new failures: https://bugs.webkit.org/show_bug.cgi?id=136124