1005922 - IonMonkey: Remove bailing on NewObject/NewArray during arguments usage analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

@Brian: does it make sense to do this for NewObject/NewArray? Else ArgumentsUsageAnalysis will always fail when there is something alike in the code.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 8417409 [details]
Patch

Yes, we should fix this, though this patch needs some more work.  jsop_initelem_array will break I think (obj->resultTypeSet()->getObject(0)) and I think we should have an idiomatic way to create unknown values during analysis, e.g. new MUnknownValue() which MOZ_CRASH()'es during lowering.

Comment 3

[Hannes Verschore [:h4writer]]

Opening up as mentored bug.

ArgumentsUsageAnalysis is a pass that runs to check if the arguments are used in a script. This runs quite early when there is (almost) no Type Information present. For the most part that isn't a problem, since we only run the first pass (IonBuilder). Now I identified two places where this does make a difference (jsop_newarray and jsop_newobject), since we abort because of that. In both cases we actually don't need to abort. We can just generate an instruction that would only be allowed during analysis and MOZ_CRASH's during lowering. To make sure we only use it during analysis.

This bug would include doing the following:
1) Create a MIR instruction MUnknownValue (in js/src/jit/MIR.h)
2) During lowering let it call MOZ_CRASH (in js/src/jit/Lowering.cpp)
3) Update jsop_newobject and jsop_newarray (in js/src/jit/IonBuilder.cpp) to emit MUnknownValue during the ArgumentsUsageAnalysis pass.

Comment 4

[Hannes Verschore [:h4writer]]

This would make us go faster on scripts that use fun.apply(, arguments) and [] or {} in a function. Since currently we need to assume arguments are actually used. This would fix it.

When there are questions or if you need help. You can contact me on irc in #ionmonkey or #jsapi

Comment 5

[Inanc Seylan]

Hi, 

I am interested in this bug and I did some progress. When we define this new instruction MUnkownValue, the class ParallelSafetyVisitor ends up being abstract. How should one classify MUnknownValue there? Safe, unsafe, etc.?

Cheers

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Inanc Seylan from comment #5)
> Hi, 
> 
> I am interested in this bug and I did some progress. When we define this new
> instruction MUnkownValue, the class ParallelSafetyVisitor ends up being
> abstract. How should one classify MUnknownValue there? Safe, unsafe, etc.?
> 
> Cheers

Thanks for volunteering.

This MUnknownValue should only be used by ArgumentsUsageAnalysis. Which is a early pass that only analyzes for certain properties. We don't want this value to live during normal IonMonkey code creation or IonMonkey Parallel compilation.

So we should classify it as Unsafe.

Comment 7

[Inanc Seylan]

Attachment: bug1005922.patch

Here's a first patch.

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8469874 [details] [diff] [review]
bug1005922.patch

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

Thanks for this patch!

There are some things that needs to get fixed first:
- jsop_initelem_array and jsop_initprop take MNewArray and MNewObject unconditionally. We now need to support MUnknownValue in those cases too.
For jsop_initelem_array we can just set "needStub = true". For jsop_initprop we can do the same as if we don't have a "shape".
- in jsop_newarray we still abort for unknown properties (during analysis). We can use this UnknownValue there too.

After these are fixed, you can test your implementation by running the jit-tests:
- cd mozilla-central/js/src
- python jit-test/jit_test.py --ion $PATH_TO_COMPILED_JS_EXECUTABLE
This should run the test harness and show possible errors.

::: js/src/jit/IonBuilder.cpp
@@ +5510,5 @@
> +        if (info().executionMode() == ArgumentsUsageAnalysis) {
> +            MUnknownValue *unknown = MUnknownValue::New(alloc());
> +            current->add(unknown);
> +            current->push(unknown);
> +            return unknown;

This should be "return true;", since we succeeded.

@@ +5550,5 @@
> +        if (info().executionMode() == ArgumentsUsageAnalysis) {
> +            MUnknownValue *unknown = MUnknownValue::New(alloc());
> +            current->add(unknown);
> +            current->push(unknown);
> +            return unknown;

This should be "return true;"

::: js/src/jit/Lowering.cpp
@@ +3643,5 @@
>  
> +bool
> +LIRGenerator::visitUnknownValue(MUnknownValue *ins)
> +{
> +    MOZ_CRASH("Can not lower unknown value");

Nit: can you add a "." at the end of the sentence.

::: js/src/jit/MIR.h
@@ +10816,5 @@
> +class MUnknownValue : public MNullaryInstruction
> +{
> +  public:
> +    INSTRUCTION_HEADER(UnknownValue)
> +

I forgot that we do need an constructor. In that constructor we need to set the type of MUnknownValue. In this case this is MIRType_Value.
So we need to do there:
setResultType(MIRType_Value);

Comment 9

[Till Schneidereit [:till]]

Hey Inanc, are you still working on this?

Comment 10

[Inanc Seylan]

(In reply to Till Schneidereit [:till] from comment #9)
> Hey Inanc, are you still working on this?

Yes, still interested; sorry I was a bit busy lately. I have rebased the patch so far on inbound but I am getting the following error during configure. Is there any way to avoid this?

==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /home/seylan/workspace/mozilla/inbound/config/external/zlib/moz.build

The error was triggered on line 10 of this file:

    OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ['ValueError: Only lists can be appended to lists.\n']

Comment 11

[Inanc Seylan]

(In reply to Inanc Seylan from comment #10)
> (In reply to Till Schneidereit [:till] from comment #9)
> > Hey Inanc, are you still working on this?
> 
> Yes, still interested; sorry I was a bit busy lately. I have rebased the
> patch so far on inbound but I am getting the following error during
> configure. Is there any way to avoid this?
> 

Ok rerunning autoconf2.13 solved the problem.

Comment 12

[Inanc Seylan]

Attachment: bug1005922.patch

I think it makes sense that you look at the current state of the patch. The test TypedObject/jit-read-u16-from-mdim-array.js fails (segmentation fault) and gdb backtrack does not give me an useful info.

python jit-test/jit_test.py --ion build-debug/dist/bin/js jit-read-u16-from-mdim-array.jsFAIL - TypedObject/jit-read-u16-from-mdim-array.js
[1|1|0|0] 100% ======================================================>|   1.2s
FAILURES:
    --ion-eager --ion-offthread-compile=off /home/seylan/workspace/mozilla/inbound/js/src/jit-test/tests/TypedObject/jit-read-u16-from-mdim-array.js
TIMEOUTS:

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8478949 [details] [diff] [review]
bug1005922.patch

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

Looks good. A few comments:

::: js/src/jit/IonBuilder.cpp
@@ +5681,5 @@
>      // Make sure that arrays have the type being written to them by the
>      // intializer, and that arrays are marked as non-packed when writing holes
>      // to them during initialization.
>      bool needStub = false;
> +    if (obj->isNewObject()) {

The bug is here. I assumed this would always be MNewObject, but that is wrong. It can also be something else. So you will have to restructure this to:

if (obj->isUnknownValue()) {
    needstub = true;
} else {
    types::TypeObjectKey *initializer = obj->resultTypeSet()->getObject(0);
    ...
}

@@ +5692,5 @@
> +            if (!TypeSetIncludes(elemTypes.maybeTypes(), value->type(), value->resultTypeSet())) {
> +                elemTypes.freeze(constraints());
> +                needStub = true;
> +            }
> +        } 

Style nit: trailing whitespace.

@@ +5696,5 @@
> +        } 
> +    } else if (obj->isUnknownValue()) {
> +        MCallInitElementArray *store = MCallInitElementArray::New(alloc(), obj, GET_UINT24(pc), value);
> +        current->add(store);
> +        return resumeAfter(store);

Like I showed a little bit higher. Just setting needStub to true is better here ;)

Comment 14

[Inanc Seylan]

Attachment: bug1005922.patch

Ok, all JS tests are green with the changes. Just a slight remark about IonBuilder::jsop_initprop -- there were three places in the method that JSOP_NEWINIT became an MNewObject without preconfigured properties. I tried avoid multiple copies of the same code by using some boolean variables.

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 8479696 [details] [diff] [review]
bug1005922.patch

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

Looks already good. There are a few style changes required by our guidelines and I suggested another name for the becomesNewObj.

::: js/src/jit/IonBuilder.cpp
@@ +5683,5 @@
>      // to them during initialization.
>      bool needStub = false;
> +    if (obj->isUnknownValue())
> +        needStub = true;
> +    else {

Style nit: if the "else" has { }, the "if" also needs { } around the body.

@@ +5769,5 @@
> +        shape = templateObject->lastProperty()->searchLinear(NameToId(name));
> +
> +        if (!shape)
> +            becomesNewObj = true;
> +    }

Good call. That would indeed be better!
- Can you call it "useSlowPath?
- Here the if also needs {} around the body.
- Can you remove the newline between "templateObject" and "shape" in de declaration and assignation (just personal taste)

@@ +5777,1 @@
>                                        &obj, name, &value, /* canModify = */ true))

Can you split this into two?

if (PropertyWrite...) {
    useSlowPath = true;
}

if (useSlowPath) {
    ...
}

Note: you'll need to use {} with the first "if" because the condition spans multiple lines. (Which our style guideline dictates).

Comment 16

[Inanc Seylan]

Attachment: bug1005922.patch

Ok, I integrated the requested changes. Hope it fits the guidelines now. Thanks again for your help Hannes.

Comment 17

[Hannes Verschore [:h4writer]]

Comment on attachment 8482245 [details] [diff] [review]
bug1005922.patch

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

This looks good. Thanks!
There is only one small style nit left:
when we have
> if () {
>     ...
> }
> else {
>     ...
> }

we put the "}" and "else {" on the same line.
So like:

> if (...) {
>    ...
> } else {
>    ...
> }

There are two places in IonBuilder.cpp that has this problem. Can you adjust those and add the new patch? You can r+ yourself and say "carries over r+ from previous patch".
Thanks!

Comment 18

[Inanc Seylan]

Attachment: bug1005922.patch

carries over r+ from previous patch

Comment 19

[Hannes Verschore [:h4writer]]

Thanks for the patch. I just landed it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/72ec272d143d

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G debug emulator-kk bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/280ac54e2dd8

https://tbpl.mozilla.org/php/getParsedLog.php?id=47820019&tree=Mozilla-Inbound

Comment 21

[Hannes Verschore [:h4writer]]

Ryan gave permission to reland. Since this is an intermediate failure. Possible not caused by this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac936775ecb6

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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