Bugzilla

(In reply to Ted Campbell [:tcampbell] from comment #46)
> Out of curiosity, do IDs need to be unique for script clones (eg CloneFunctionAndScript)?

Hmm, yes they do.  We need to be able to map the ID back to the Debugger.Script it is associated with, and if there are multiple Debugger.Scripts which it could map to then we don't know what code is actually executing.  If the script's identifying information is directly embedded in the bytecode, in a way that the value doesn't change when the script is cloned, then it seems impossible for downstream code to associate the ID with a particular Debugger.Script.  Moreover, if the identifying information includes a script source ID, and the source of the cloned script is different from the source of the original script, the identifying information in the cloned script will be wrong.

So, given the existence of script cloning (which I'd totally forgotten about earlier) I don't think we can get the property of providing a unique ID for the executing script without a new opcode.  I know JSOP_INSTRUMENTATION_SCRIPT_ID is a little awkward, but it's designed to avoid bloating JSScript and to minimize the extra VM complexity, i.e. no new weak maps.  I can remove the TLS access in IonBuilder and just abort the builder if there isn't an instrumentation ID set on the script, but it also seems kind of nice to use a JSContext and behave uniformly across the different execution engines.

FWIW the TLS access in IonBuilder is necessary because IonBuilder doesn't normally have a JSContext, but the main reason for that is historical --- several years ago (bug 785905) I moved MIR construction off thread, a change which got backed out pretty quickly due to its complexity, introduction of new race conditions, and lack of meaningful speedup on benchmarks.  While not having a JSContext makes it easy to avoid calling methods that have side effects, it also leads to cumbersome code like having to store template objects in baseline ICs instead of allocating them in IonBuilder.  It might be worth letting IonBuilder always have a JSContext again, so that things like this could be streamlined.

(In reply to Ted Campbell [:tcampbell] from comment #46)
> Out of curiosity, do IDs need to be unique for script clones (eg CloneFunctionAndScript)?

Hmm, yes they do.  We need to be able to map the ID back to the Debugger.Script it is associated with, and if there are multiple Debugger.Scripts which it could map to then we don't know what code is actually executing.  If the script's identifying information is directly embedded in the bytecode, in a way that the value doesn't change when the script is cloned, then it seems impossible for downstream code to associate the ID with a particular Debugger.Script.  Moreover, if the identifying information includes a script source ID, and the source of the cloned script is different from the source of the original script, the identifying information in the cloned script will be wrong.

So, given the existence of script cloning (which I'd totally forgotten about earlier) I don't think we can get the property of providing a unique ID for the executing script without a new opcode.  I know JSOP_INSTRUMENTATION_SCRIPT_ID is a little awkward, but it's designed to avoid bloating JSScript, to minimize the extra VM complexity (i.e. no new weak maps), and (edited) to provide a simple interface for Debugger users whose implementation is straightforwardly correct.  I can remove the TLS access in IonBuilder and just abort the builder if there isn't an instrumentation ID set on the script, but it also seems kind of nice to use a JSContext and behave uniformly across the different execution engines.

FWIW the TLS access in IonBuilder is necessary because IonBuilder doesn't normally have a JSContext, but the main reason for that is historical --- several years ago (bug 785905) I moved MIR construction off thread, a change which got backed out pretty quickly due to its complexity, introduction of new race conditions, and lack of meaningful speedup on benchmarks.  While not having a JSContext makes it easy to avoid calling methods that have side effects, it also leads to cumbersome code like having to store template objects in baseline ICs instead of allocating them in IonBuilder.  It might be worth letting IonBuilder always have a JSContext again, so that things like this could be streamlined.