Bug 135578 - The JIT should cache property lookup misses.
Summary: The JIT should cache property lookup misses.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
Depends on: 135635
  Show dependency treegraph
Reported: 2014-08-04 15:57 PDT by Andreas Kling
Modified: 2014-11-10 19:10 PST (History)
4 users (show)

See Also:

Pork in wogress (5.98 KB, patch)
2014-08-04 15:57 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Kinda works (7.46 KB, patch)
2014-08-05 10:10 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (10.89 KB, patch)
2014-08-05 15:45 PDT, Andreas Kling
ggaren: review+
Details | Formatted Diff | Diff
Patch (14.21 KB, patch)
2014-11-10 16:59 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2014-11-10 17:09 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2014-08-04 15:57:27 PDT
Because why not?
Comment 1 Andreas Kling 2014-08-04 15:57:44 PDT
Created attachment 235987 [details]
Pork in wogress
Comment 2 Andreas Kling 2014-08-05 10:10:16 PDT
Created attachment 236033 [details]
Kinda works
Comment 3 Andreas Kling 2014-08-05 15:45:50 PDT
Created attachment 236058 [details]
Comment 4 Geoffrey Garen 2014-08-05 16:00:32 PDT
Comment on attachment 236058 [details]

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


> LayoutTests/js/regress/script-tests/undefined-property-access.js:3
> +    var someGlobal = 0;

Ruh roh. This is not a global, so this test might get over-optimized.
Comment 5 Andreas Kling 2014-08-05 16:10:58 PDT
Committed r172099: <http://trac.webkit.org/changeset/172099>
Comment 6 Filip Pizlo 2014-08-05 19:20:39 PDT
I believe that this change needs to be reverted until we get it right: it needs some real tests, a clear story for how the new caching style communicates to the DFG, a clear story for polymorphic cases, and a clear story for prototype chain caching safety checks.


For starters, this should have included a test for the counterexamples:

- We cached a miss but the field appeared on the prototype chain.
- We cached a miss but the field appeared on the object itself.
- Tests for all of the counterexamples I come up with below.


It also appears that you are essentially relying on luck to get the DFG to ignore such inline caches.  This is not a good thing.  The Repatch.cpp logic needs to communicate explicitly to the logic beneath GetByIdStatus/PutByIdStatus/CallLinkStatus, so that when the DFG sees an inline cache of a certain form it either soundly inlines it, or ignores it.  In this case, you have no DFG changes to support such misses so you want the DFG to ignore it.  But you're doing this:

            *vm, codeBlock->ownerExecutable(),
            stubRoutine, structure, prototypeChain, count));

Which is an emphatic claim to the DFG that at the given chain count, there will be the property that we're looking for.  This is of course false in this case.  The correct way to do this is to introduce a new GetByIdAccess enumeration value, like GetByIdAccess::SimpleMiss


I am surprised that you put this case inside tryCacheGetByID().  It doesn't belong there.  All caching that results in a stub should funnel into tryBuildGetByIDList().  Not only is this the consistent thing to do - and consistency is crucial to reasoning about this code - but it will give you a significant speed increase.  Consider that the object either may have the field or may not.  Your patch will *only* produce a speed-up if it *always* does not have the field.  That's not what we really want.  We want a speed-up in cases where it may or may not have the field, also.

Also, it looks like you're missing out of the proxy logic in tryBuildGetByIDList().  Literally moving your code into there would give you loads of benefit.

I realize that this could be viewed as a request for improvement.  But it's extremely important for compiler and runtime optimizations to be implemented to their fullest generality on the first try.  Otherwise, future contributors will think that the optimization was done in a limited fashion on purpose.  This is very confusing and we had a nightmare with this in the baseline JIT's old inline caching.  I don't want a repeat of that disaster.


Because you implemented this separately from the existing prototype chain logic in tryBuildGetByIDList(), you missed some safety checks, like:

    if (slot.slotBase() != baseValue) {
        if (typeInfo.prohibitsPropertyCaching() || structure->isDictionary())
            return GiveUpOnCache;

That's a pretty bad bug.

I think that it's best to revert this change because it introduces a lot of regressions.  I think it's better to revert and fix those before relanding this rather than filing a ton of bugs about all of them.
Comment 7 WebKit Commit Bot 2014-08-05 19:27:40 PDT
Re-opened since this is blocked by bug 135635
Comment 8 Andreas Kling 2014-11-10 16:59:42 PST
Created attachment 241319 [details]

Moved the logic to tryBuildGetByIDList(), duplicated less code, and added more test coverage.
Comment 9 Filip Pizlo 2014-11-10 17:03:27 PST
Comment on attachment 241319 [details]

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

> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:180
> +                case GetByIdAccess::SimpleMiss:

This seems wrong - you should put it along with the CustomGetter/WatchedStub because you haven't written the code that would make the DFG support this.  So, currently, if you end up in the DFG with this kind of cache you might have a bad time.
Comment 10 Andreas Kling 2014-11-10 17:09:48 PST
Created attachment 241321 [details]

Fixed per Filip's comment.
Comment 11 Filip Pizlo 2014-11-10 17:22:46 PST
Comment on attachment 241321 [details]

It seems like it would be good to have a test for the case that you just fixed in this patch. :-)
Comment 12 WebKit Commit Bot 2014-11-10 19:10:16 PST
Comment on attachment 241321 [details]

Clearing flags on attachment: 241321

Committed r175846: <http://trac.webkit.org/changeset/175846>
Comment 13 WebKit Commit Bot 2014-11-10 19:10:21 PST
All reviewed patches have been landed.  Closing bug.