841499 - Allow calling SetObjectElementOperation with explicit script and pc arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

This is needed for baseline.  SetObjectElementOperation updates ScriptAnalysis' arrayWriteHole flag, but not when it's in an Ion activation.  This is because it needs to use TypeScript::GetPcScript to get the script and pc, which doesn't work inside an ion activation.

This is OK for Ion becuase Ion was never expected to generate analysis info, just use it.  It's not OK for baseline because baseline needs to record this for later use by Ion.

To allow baseline to do SetElems and set this appropriately, we need SetObjectElementOperation to accept an explicit script and pc, which can be used to update this flag even if inside an ion activation.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Patch

Comment 2

[Jim Blandy :jimb]

Comment on attachment 714052 [details] [diff] [review]
Patch

Review of attachment 714052 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Sorry for the nitpicking; hopefully there's something of value in there.

::: js/src/jsinterp.cpp
@@ +3581,5 @@
>      return SetObjectElementOperation(cx, obj, id, value, strict);
>  }
>  
>  bool
> +js::SetObjectElementWithScriptAndPC(JSContext *cx, HandleObject obj, HandleValue index,

If it works, I'd prefer this as an overloading of SetObjectElement.

@@ +3591,5 @@
> +    RootedValue indexval(cx, index);
> +    if (!FetchElementId(cx, obj, indexval, &id, &indexval))
> +        return false;
> +    return SetObjectElementOperation(cx, obj, id, value, strict,
> +                                     script, uint32_t(pc - script->code));

It's a pity that SetObjectElementWithScriptAndPC converts the pc to an offset, and then passes it to SetObjectElementOperation which converts it back to a pointer.

::: js/src/jsinterpinlines.h
@@ +902,5 @@
>      if (obj->isArray() && JSID_IS_INT(id)) {
>          uint32_t length = obj->getDenseInitializedLength();
>          int32_t i = JSID_TO_INT(id);
> +        if ((uint32_t)i >= length) {
> +            if (maybeRootedScript || !cx->fp()->beginsIonActivation()) {

(It might be nicer to use maybeRootedScript in both conditionals, or maybeScript in both; but using one and the other made me wonder whether it could make a difference.)

I would appreciate a comment here, to the effect of:

"In an Ion activation, getPcScript doesn't work. For non-baseline activations that's okay, because they don't need to generate analysis info. However, baseline Ion activations must do so; they pass the script as an argument."

@@ +903,5 @@
>          uint32_t length = obj->getDenseInitializedLength();
>          int32_t i = JSID_TO_INT(id);
> +        if ((uint32_t)i >= length) {
> +            if (maybeRootedScript || !cx->fp()->beginsIonActivation()) {
> +                JSScript *script = NULL;

Rather than having 'maybeScript', 'maybeRootedScript', and 'script', we could just pass maybeRootedScript.address() to types::TypeScript::GetPcScript, and then use that everywhere.

And if we passed in a jsbytecode *pc instead of bytecodeOffset, then there'd be no need for the 'if (maybeScript)' branch at all.

Comment 3

[Kannan Vijayan [:djvj]]

Your suggestions cleaned up that logic quite a bit actually.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7da7e33bf62

Waiting for tbpl before closing.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7da7e33bf62