1146949 - Can/should we kill CloneObjectLiteral?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we use CloneObjectLiteral in only one case: to clone objects hanging off a script in CloneScript.

Objects can hang off a script for various reasons, but one of those reasons is JSOP_OBJECT.

The thing is, a JSOP_OBJECT object actually has values for its properties.  But CloneObjectLiteral completely ignores the values.  So right now cloning a script containing JSOP_OBJECT is broken.  Luckily, we never do that at the moment, but if we want to move to a no-compileAndGo world, there's no reason that wouldn't be able to happen, afaict.

We have a DeepCloneObjectLiteral, which seems to be a bit slower at first glance (e.g. it doesn't just copy the shape) but copies the values properly.  So it seems like we should be able to just DeepCloneObjectLiteral in CloneScript and have cloning of scripts with JSOP_OBJECT actually work...

It seems to me like the two options here are either to make this change or to keep some notion of compileAndGo, defined as "script is never cloned, only executed once" or something...

Comment 1

[Brian Hackett [Laid off!]]

JSOP_OBJECT is also tricky in that the object associated with the JSOP_OBJECT is the actual object which will be used, and can be modified freely by user scripts.  So scripts containing JSOP_OBJECT can only be executed once and can't be cloned, unless the cloneSingletons() compartment option is set (which it never is afaik).  Now, we could just reparse from the original source if we end up in the unfortunate position of needing to execute a script with JSOP_OBJECT a second time.  That would require only emitting JSOP_OBJECT if the source is retrievable, but that restriction should be fine.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Right, so that's the other question.  If we want to allow cloning scripts with JSOP_OBJECT, we basically need to always act like cloneSingletons() is set or ... something.

It would be good to understand what the desired end goal here is.  For a start, should any JSScript be clonable or only some?

This affects how we want to proceed with compileAndGo removal, precisely because of the interaction with JSOP_OBJECT.

Comment 3

[Brian Hackett [Laid off!]]

Well, JSOP_OBJECT isn't the only place where we have to deal with the complexity of scripts running multiple times which we didn't expect to.  I don't think we should deoptimize just because the script *might* run again in the future; this has both performance and memory costs.  If we have a mechanism that ensures particular scripts will only execute once, we can remove the stuff that has accumulated to cope with the running-multiple times case, like cloneSingletons(), the OBJECT_FLAG_RUNONCE_INVALIDATED flag, and this hasBeenCloned() bit on lazy and normal scripts.  This mechanism would just be something like:

- Keep a bit on the script indicating it only expects to run once, and a bit indicating whether it has run.
- When running a script that only expects to run once, check the bit.
- If unset, set it, and run the script.
- If set, reparse the script from its original source, then run that new script instead.

There are some more refinements that could be made here.  It is kind of complicated though.  In the interim I think it would be fine to have a flag (runOnce or something) that is just used during parsing and decides whether to emit JSOP_OBJECT, singleton functions, etc.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I'd been thinking of something pretty much like your mechanism, but with cloning instead of reparsing...  Especially if we ever make this startup cache thing happen.

As for the runOnce bit, we have that right now, kinda.  It's called compileAndGo.  The only remaining consumers in the tree are the one that decides to output JSOP_OBJECT, the one that decides to do singleton functions, and two places that check that we're either not cloning a compileAndGo script or are cloning it into a global (the former would need to be the way it works, clearly).

So maybe we should just restrict compileAndGo to toplevel scripts, rename it to runOnce to make it clearer what it means, flag when a runOnce script has run, and throw from execute entry points if trying to run it again.  Cloning a runOnce script would also throw.  We'd need to make bce::isRunOnceLambda() return false if the original toplevel script was not runOnce; we can propagate that down to child bce's.

Seem plausible?

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Brian Hackett (:bhackett) from comment #3)
> - If set, reparse the script from its original source, then run that new
> script instead.

I see multiple issues with this approach and the start-up cache, which is that the new script will have lazy scripts, and not the delazified one that we want to save in the start-up cache.

Would it be possible to just re-parse the object literal and not the entire script when encoding / cloning the object?

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  I'm just going to implement the runOnce thing for now, which means we will never be cloning things with JSOP_OBJECT.  But we should really decide what we're doing here, since it affects whether bug 1143269 and bug 1143270 need fixing.

Comment 7

[Boris Zbarsky [:bzbarsky]]

CloneObjectLiteral got killed in bug 1172943.