Bug 133398 - It's pretty weird that a node that has a side-effect that jettisons the calling code will return normally, and more DFG nodes may be executed, before we finally exit at the invalidation point at the top of the next bytecode instruction
Summary: It's pretty weird that a node that has a side-effect that jettisons the calli...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 128073
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-29 22:25 PDT by Filip Pizlo
Modified: 2014-05-30 14:27 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-05-29 22:25:24 PDT
This doesn't cause problems, yet.  But it could cause problems in the future.

It seems that we want the invalidation point insertion phase to distinguish between nodes that perform observable effects and nodes that just fire watchpoints.  Nodes that fire watchpoints but do not cause observable effects could have an immediate invalidation point just after them, while nodes that cause observable effects will have a deferred invalidation point at the top of the next bytecode instruction just as we do now.

We could also require that:

- Checking nodes cannot have observable effects.  We already require this.
- Checking nodes with invisible effects that cause jettison act as their own invalidation points.

Part of what is hard about this is that clobberize() doesn't capture whether effects are observable.  Perhaps we should have a separate thing that answers this question.  If we did have such a separate thing, then the first solution would be the easiest to implement.
Comment 1 Filip Pizlo 2014-05-29 22:52:35 PDT
It feels like a good way of modeling this is to have the notion of "future possible" state.  It is clobbered until the next invalidation point, which unclobbers it...
Comment 2 Filip Pizlo 2014-05-29 22:59:18 PDT
What if the AI modeled this explicitly?  We could say that a clobbering node put the whole abstract state in purgatory, which lasts until the next InvalidationPoint.
Comment 3 Filip Pizlo 2014-05-29 23:03:12 PDT
(In reply to comment #2)
> What if the AI modeled this explicitly?  We could say that a clobbering node put the whole abstract state in purgatory, which lasts until the next InvalidationPoint.

Then, if we had an ArrayifyToStructure and no subsequent InvalidationPoint then we would believe that the whole world got clobbered.  That's not quite what we want.  We want to be able to say that we know for sure that the node being arrayified will now have the structure to which we are arrayifying.

Perhaps the solution here is to have the notion of a "clobbered" entry in the structure set...
Comment 4 Filip Pizlo 2014-05-29 23:25:36 PDT
Part of the problem is that invalidation points are separate from the side-effecting operation that caused them.

This works because we allow for only very limited code motion.  It's only legal to hoist to the top of a bytecode instruction boundary.

Maybe that's fine, and we can basically keep things the way they are.

If we believe that it's bad to have a region of code between when a side effect happened and when we invalidate, then we should probably do the following:

- Merge MovHints with Nodes.  A Node may claim to have a MovHint inside it.  No separate MovHint node.
- Side-effecting nodes will have their own invalidation point inside them, which will do an exit that follows the implicit MovHint.

You could imagine making this work by having a late lowering from Nodes with implicit MovHints and implicit InvalidationPoints to Node+MovHint+InvalidationPoint.

Meh.  I'm not sure that this is profitable in any way over what we have now.  The only thing wrong with what we have now is that a checking node can cause state that diverges from what the AI would believe, and we only converge back at the end of the bytecode instruction, at the next InvalidationPoint.
Comment 5 Filip Pizlo 2014-05-30 14:27:52 PDT
I'm going to fix this as part of https://bugs.webkit.org/show_bug.cgi?id=128073.