Bug 135578 – The JIT should cache property lookup misses.

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

[Andreas Kling]

Because why not?

[Andreas Kling]

Created attachment 235987 [details]
Pork in wogress

[Andreas Kling]

Created attachment 236033 [details]
Kinda works

[Andreas Kling]

Created attachment 236058 [details]
Patch

[Geoffrey Garen]

Comment on attachment 236058 [details]
Patch

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

r=me

> 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.

[Andreas Kling]

Committed r172099: <http://trac.webkit.org/changeset/172099>

[Filip Pizlo]

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.

REAL TESTS

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.

INTEGRATION WITH THE DFG

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:

        list->addAccess(GetByIdAccess(
            *vm, codeBlock->ownerExecutable(),
            GetByIdAccess::SimpleInline,
            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

POLYMORPHIC CASES

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.

PROTOTYPE CHAIN SOUNDNESS

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 135635

[Andreas Kling]

Created attachment 241319 [details]
Patch

Moved the logic to tryBuildGetByIDList(), duplicated less code, and added more test coverage.

[Filip Pizlo]

Comment on attachment 241319 [details]
Patch

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.

[Andreas Kling]

Created attachment 241321 [details]
Patch

Fixed per Filip's comment.

[Filip Pizlo]

Comment on attachment 241321 [details]
Patch

It seems like it would be good to have a test for the case that you just fixed in this patch. :-)

[WebKit Commit Bot]

Comment on attachment 241321 [details]
Patch

Clearing flags on attachment: 241321

Committed r175846: <http://trac.webkit.org/changeset/175846>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.