Bug 142920 - Observably effectful nodes in DFG IR should come last in their bytecode instruction (i.e. forExit section), except for Hint nodes
Summary: Observably effectful nodes in DFG IR should come last in their bytecode instr...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
Depends on:
Blocks: 142921 142924 141174
  Show dependency treegraph
Reported: 2015-03-20 14:04 PDT by Filip Pizlo
Modified: 2015-03-20 16:30 PDT (History)
11 users (show)

See Also:

the patch (10.30 KB, patch)
2015-03-20 14:25 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (10.30 KB, patch)
2015-03-20 14:37 PDT, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-03-20 14:04:54 PDT
Observably effectful, n.: If we reexecute the bytecode instruction after this node has executed, then something other than the bytecode instruction's specified outcome will happen.

We almost never had observably effectful nodes except at the end of the bytecode instruction.  The exception is a lowered transitioning PutById:

PutStructure(@o, S1 -> S2)
PutByOffset(@o, @o, @v)

The PutStructure is observably effectful: if you try to reexecute the bytecode after doing the PutStructure, then we'll most likely crash.  The generic PutById handling means first checking what the old structure of the object is; but if we reexecute, the old structure will seem to be the new structure.  But the property ensured by the new structure hasn't been stored yet, so any attempt to load it or scan it will crash.

Intriguingly, however, none of the other operations involved in the PutById are observably effectful.  Consider this example:

PutByOffset(@o, @o, @v)
PutStructure(@o, S1 -> S2)

Note that the PutStructure node doesn't reallocate property storage; see further below for an example that does that. Because no property storage is happening, we know that we already had room for the new property.  This means that the PutByOffset is no observable until the PutStructure executes and "reveals" the property.  Hence, PutByOffset is not observably effectful.

Now consider this:

b: AllocatePropertyStorage(@o)
PutByOffset(@b, @o, @v)
PutStructure(@o, S1 -> S2)

Surprisingly, this is also safe, because the AllocatePropertyStorage is not observably effectful. It *does* reallocate the property storage and the new property storage pointer is stored into the object. But until the PutStructure occurs, the world will just think that the reallocation didn't happen, in the sense that we'll think that the property storage is using less memory than what we just allocated. That's harmless.

The AllocatePropertyStorage is safe in other ways, too. Even if we GC'd after the AllocatePropertyStorage but before the PutByOffset (or before the PutStructure), everything could be expected to be fine, so long as all of @o, @v and @b are on the stack. If they are all on the stack, then the GC will leave the property storage alone (so the extra memory we just allocated would be safe). The GC will not scan the part of the property storage that contains @v, but that's fine, so long as @v is on the stack.
Comment 1 Filip Pizlo 2015-03-20 14:25:30 PDT
Created attachment 249135 [details]
the patch
Comment 2 Filip Pizlo 2015-03-20 14:27:17 PDT
Comment on attachment 249135 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1785
> -        }
>          m_insertionSet.insertOutOfOrderNode(

This doesn't have to insertOUtOfOrderNode anymore.  I'll try that...
Comment 3 Filip Pizlo 2015-03-20 14:37:44 PDT
Created attachment 249136 [details]
the patch

Revised patch with more simplification.
Comment 4 Geoffrey Garen 2015-03-20 14:46:20 PDT
Comment on attachment 249136 [details]
the patch

r=me too
Comment 5 Filip Pizlo 2015-03-20 15:04:57 PDT
The patch that I will land will kill some dead code that mlam added, per his request.
Comment 6 Filip Pizlo 2015-03-20 16:30:00 PDT
Landed in http://trac.webkit.org/changeset/181817