Closed Bug 1005922 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: h4writer, Assigned: inanc.seylan, Mentored)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(2 files, 4 obsolete files)

No description provided.
Attached file 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.
Attachment #8417409 - Flags: feedback?(bhackett1024)
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.
Attachment #8417409 - Flags: feedback?(bhackett1024) → feedback+
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.
Whiteboard: [good first bug][mentor=h4writer][lang=c++]
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
Mentor: hv1989
Whiteboard: [good first bug][mentor=h4writer][lang=c++] → [good first bug][lang=c++]
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
(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.
Attached patch bug1005922.patch (obsolete) — Splinter Review
Here's a first patch.
Attachment #8469874 - Flags: review?(hv1989)
Assignee: nobody → inanc.seylan
Status: NEW → ASSIGNED
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);
Attachment #8469874 - Flags: review?(hv1989)
Hey Inanc, are you still working on this?
Flags: needinfo?(inanc.seylan)
Flags: needinfo?(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']
(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.
Attachment #8469874 - Attachment is obsolete: true
Attached patch bug1005922.patch (obsolete) — Splinter Review
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:
Attachment #8478949 - Flags: review?(hv1989)
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 ;)
Attachment #8478949 - Flags: review?(hv1989)
Attachment #8478949 - Attachment is obsolete: true
Attached patch bug1005922.patch (obsolete) — Splinter Review
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.
Attachment #8479696 - Flags: review?(hv1989)
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).
Attachment #8479696 - Flags: review?(hv1989)
Attachment #8479696 - Attachment is obsolete: true
Attached patch bug1005922.patch (obsolete) — Splinter Review
Ok, I integrated the requested changes. Hope it fits the guidelines now. Thanks again for your help Hannes.
Attachment #8482245 - Flags: review?(hv1989)
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!
Attachment #8482245 - Flags: review?(hv1989) → review+
Attachment #8482245 - Attachment is obsolete: true
Attached patch bug1005922.patchSplinter Review
carries over r+ from previous patch
Attachment #8486269 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/ac936775ecb6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.