Closed Bug 671360 Opened 13 years ago Closed 13 years ago

This JSOP_BLOCKCHAIN/NULLBLOCKCHAIN stuff is getting just a little ridiculous

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 659577

People

(Reporter: jorendorff, Unassigned)

Details

(Whiteboard: [js-triage-done])

See bug 646968 comment 25 and my reply in #26.

It works, but those NOPs are pretty silly. Suggestions in #26 and #27:

  - use different ops for "after this point, the block chain is X"
    and "at the preceding instruction, the block chain is X"

  - call GetScopeChainFast/GetBlockChainFast iff we are sure the annotation
    op is there, following the instruction at pc

  - assert that it is there, instead of checking and providing a fallback path

Then the NOPs can be removed.

Ideally though we would never do a linear scan of the bytecode just to figure out the current scope. So a more thoroughgoing cleanup would also be welcome.

Triage: No immediate action needed. Hoping to get his fixed by cc-ing right-thinking individuals and putting a FIXME comment in the code.
It might make sense to revert the patch that introduced these opcodes and just initialize the blockchain lazily, like we do with a lot of other stuff. I think Brian even had a patch for that a long time ago.
So, its kinda my fault for pushing to remove fp->blockChain (to get it off the call path) instead of just making fp->blockChain lazily-initialized.  I've talked about this with Bill and basically the best thing might be to just revert the patch and go back to having an fp->blockChain that gets dynamically updated.

In a shinier future, we will decide a priori what activation objects (Call, DeclEnv, Block, Arguments) need to be created and eagerly create these objects and ensure that we never need to lazily create an activation object that didn't get created eagerly.  (evalInFrame is, of course, the only hardship but we have a plan; jimb approves.)  With this, there won't be any need for a blockChain, but that's farther off, so perhaps we can revert first?
(In reply to comment #2)
> In a shinier future, we will decide a priori what activation objects (Call,
> DeclEnv, Block, Arguments) need to be created and eagerly create these
> objects and ensure that we never need to lazily create an activation object
> that didn't get created eagerly.  (evalInFrame is, of course, the only
> hardship but we have a plan; jimb approves.)

Not to sidetrack too much, but won't this also have problems with foo.arguments accesses?  Even though these won't alias the function's actuals if the function does not use 'arguments' directly, it still seems the resulting arguments objects will need to be stored or at least associated with the frame.
(In reply to comment #3)
Right, so the hard case of course is when foo.arguments is called and foo did not eagerly create an arguments object and this is non-strict code.  In this case there is no specified semantics, but the web does use this.  We discovered by probing that v8 chooses to break things a bit: v8 lets you read and write properties of f.arguments, but any values stored are not propagated to the formals of the function.  Thus, IIUC, all we need to be able to do is create an arguments object that has a copy of the data (using the same reconstruction data that let's us bail) and not worry about aliasing formals etc.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.