Bug 133426 – [ftlopt] DFG should use its own notion of JSValue, which we should call FrozenValue, that will carry around a copy of its structure

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

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 232322 [details]
it begins!

[Filip Pizlo]

Created attachment 232328 [details]
more

[Filip Pizlo]

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.

[Oliver Hunt]

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.

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 232492 [details]
more!

[Filip Pizlo]

Created attachment 232504 [details]
more!!

[Filip Pizlo]

Created attachment 232508 [details]
just a tiny bit more

This is going to be such a surgical and incremental change.

[Filip Pizlo]

Created attachment 232654 [details]
it compiles!

[Filip Pizlo]

Created attachment 232666 [details]
passes tests

I still need to make sure that I didn't break performance.

[Filip Pizlo]

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

[Geoffrey Garen]

Comment on attachment 232798 [details]
the patch

r=me

[Filip Pizlo]

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