Bug 133426

Created attachment 232322 [details] it begins!

Created attachment 232328 [details] more

I initially started working on this because I believed that it would be absolutely necessary for making eager watchable structure watching sound. I no longer believe it's needed for soundness, but it is worth pursuing anyway because it'll make debugging the DFG a lot easier. This isn't needed for soundness so long as we always try to watch the structures of constants as soon as they are introduced into the abstract interpreter, inside of AbstractValue::set(). It's of course possible for weirdness: - The value could start with an unwatchable structure and then transition to a watchable structure. Fortunately, we will assume that the structure is TOP if it is unwatchable. So, this would mean that during the same abstract interpretation cycle, we will still believe that the structure is TOP. In future cycles, we may believe that the structure is more specific, but that's OK - it's always fine to continue optimizing code. - The value could start with some watchable structure and then it transitions to another watchable structure. This would mean that the first structure's watchpoint would have gotten fired, and the code would get invalidated before installation anyway. Of course, this may be all fine and good, but it's confusing as heck. Any bugs in this would be impossible to debug because of the race conditions involved. So, it's just a whole lot easier lock in the structure of every value we see.

Comment on attachment 232328 [details] more View in context: https://bugs.webkit.org/attachment.cgi?id=232328&action=review Wow, i really like this. I also really like the amount of red :) > Source/JavaScriptCore/runtime/JSCJSValue.cpp:195 > + out, context, (!!*this && isCell()) ? asCell()->structure() : nullptr); Our normal approach is to add that awful operator <functionptr madness>() const so that you'd only need *this && isCell(), which i think would look nicer here.

(In reply to comment #4) > (From update of attachment 232328 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232328&action=review > > Wow, i really like this. I also really like the amount of red :) > > > Source/JavaScriptCore/runtime/JSCJSValue.cpp:195 > > + out, context, (!!*this && isCell()) ? asCell()->structure() : nullptr); > > Our normal approach is to add that awful operator <functionptr madness>() const so that you'd only need *this && isCell(), which i think would look nicer here. Right, exactly, madness. That's why I didn't add it. Not-madness is better than madness.

Created attachment 232492 [details] more!

Created attachment 232504 [details] more!!

Created attachment 232508 [details] just a tiny bit more This is going to be such a surgical and incremental change.

Created attachment 232654 [details] it compiles!

Created attachment 232666 [details] passes tests I still need to make sure that I didn't break performance.

Created attachment 232798 [details] the patch Note that the change to Structure::dfgShouldWatch() is going to be landed separately: https://bugs.webkit.org/show_bug.cgi?id=133624

Comment on attachment 232798 [details] the patch r=me

Landed in http://trac.webkit.org/changeset/169795