This JSOP_BLOCKCHAIN/NULLBLOCKCHAIN stuff is getting just a little ridiculous

RESOLVED DUPLICATE of bug 659577

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 659577
7 years ago
7 years ago

People

(Reporter: jorendorff, Unassigned)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js-triage-done])

(Reporter)

Description

7 years ago
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.

Comment 2

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

Comment 4

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

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 659577
You need to log in before you can comment on or make changes to this bug.