Because why not?
Created attachment 235987 [details] Pork in wogress
Created attachment 236033 [details] Kinda works
Created attachment 236058 [details] Patch
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.
Committed r172099: <http://trac.webkit.org/changeset/172099>
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.
Re-opened since this is blocked by bug 135635
Created attachment 241319 [details] Patch Moved the logic to tryBuildGetByIDList(), duplicated less code, and added more test coverage.
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.
Created attachment 241321 [details] Patch Fixed per Filip's comment.
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. :-)
Comment on attachment 241321 [details] Patch Clearing flags on attachment: 241321 Committed r175846: <http://trac.webkit.org/changeset/175846>
All reviewed patches have been landed. Closing bug.