792108 - Use a class flag to allow objects of that class to act like |undefined| in the == and ToBoolean contexts, and remove JSRESOLVE_DETECTING

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: v1

JSRESOLVE_DETECTING, used only by |document.all|, is usually inferred from the current bytecode.  Inferred flags make it difficult to convert to new property-access APIs that will make resolveFlags an explicit argument.  The problem is that a getProperty call site implicitly passes many different resolveFlags.  We could infer flags at each call site, but then our "explicit" resolveFlags would be no better than the current implicit ones.

Permitting some objects to act like the value |undefined| in the == and ToBoolean contexts gets us most of the "benefits" of JSRESOLVE_DETECTING without its hazards and complexities.  We'd lose the ability to say |document.all| doesn't exist via "in" or "hasOwnProperty" queries and similar.  But we only implement |document.all| in quirks mode, which blunts the effect of that change.  Furthermore, every other browser says |document.all| exists via those queries, so that behavior is web-shippable.

Other browsers implement document.all using this undefined-looking mechanism;  switching to it would slightly improve cross-browser compatibility.  The attached patch implements this and is running through try now.

https://tbpl.mozilla.org/?tree=Try&rev=10d46a77b3e7

A slightly different earlier iteration passed try, and this patch builds locally, so I think this is ready for review.  Comments particularly appreciated on the DOM changes -- bz and I looked and think this flag's only needed on the document.all class, and not on the document.all helper class, but the entire "all" property mechanism is byzantine enough that a second look would be good.

This removes JSAPI, so I'll need to run it past the newsgroup before it goes in, but I'd slightly prefer to have a patch ready to go when I send the mail.  I don't think anyone else will be using JSRESOLVE_DETECTING -- we were the ones who needed it and added it -- but you never know.

Comment 1

[Luke Wagner [:luke]]

Yes, please!  We have broken document.all no less than 3 times in the last half year due to the extreme fragility of the current implementation strategy.  Here is the current bustage: bug 786801.

Comment 2

[Luke Wagner [:luke]]

(Also, for a later bug, can't we do something else about the warning-quelling so that we can remove JOF_DETECTING?  Seems like we could just sniff "if (obj.foo)" which is much simpler.)

Comment 3

[Jeff Walden [:Waldo]]

The patch needs more JIT-futzing (thanks to evilpie for pointing it out), but what's there's still reviewable in the interim -- review of a second patch atop it should be separable.

Comment 4

[Jan de Mooij [:jandem]]

For fuzzing and (shell-)testing, it would be good to add an object with that flag to the shell.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: v1.9ish checkpoint

The other-jit work seems to be increasing enough that it's probably not worth looking at the old parts in isolation.

This new patch is almost to the point of being done, but it generates incorrect results on a few tests and could still probably use some tightening up.  Still, if anyone wants to kibitz, go for it.  :-)

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 662214 [details] [diff] [review]
v1

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

r=me as far as this version goes. I had already gotten pretty far into it before the new patch came up. Will try interdiffing.

::: js/src/jsapi-tests/testObjectEmulatingUndefined.cpp
@@ +34,5 @@
> +    jsval result;
> +
> +    EVAL("if (new ObjectEmulatingUndefined()) true; else false;", &result);
> +    CHECK(JSVAL_IS_BOOLEAN(result));
> +    CHECK(JSVAL_TO_BOOLEAN(result) == false);

CHECK_SAME(result, JSVAL_FALSE) would work, right?

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Patch, v2, still not quite ready

Okay, this patch seems to work and passed try in a minimally-changed previous version.  It does still have a few issues to resolve before it can truly move forward, tho.

First, and easiest, the JSAPI-change issue mentioned before still needs to be taken up in newsgroups.

Second, there's a bit of perf difference here on some stuff.  SunSpider perf regresses by something like 0.4% or so.  Given the value of the changes here, and that this moves a step closer to removing js_InferFlags (bad bad bad for perf), maybe that's fine.  v8 unfortunately changes rather more markedly.  v8-richards goes from ~10k to ~6k most prominently, and a couple other tests regress by more-than-noise amounts as well -- not all of them, but a few of them.

As far as perf-fixing goes: Luke's all gung-ho about an inline fast path, which I guess would have to do an IsProxy() check, a proxyhandler->flags check, and finally the clasp->emulatesUndefined() check.  That's enough extra code to make inlining it set off warning flags in my head.  Or maybe the solution is to make the object-truthiness code be out-of-line?  I don't have intuition for any of this, and whatever's the right solution will take me as a relative ion novice long enough that I probably should hand this off to someone else for the last bit.

So...any takers on picking this up and ironing out the last little bits of perf regression?

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> which I guess would have to do an IsProxy() check, a proxyhandler->flags
> check, and finally the clasp->emulatesUndefined() check.

More correctly and more precisely:

* loading obj->shape_->base_->clasp
* comparing that against &ObjectProxyClass and &OuterWindowProxyClass and &FunctionProxyClass
** if not, then fast-path
* else
** a reserved-slot load, then a toPrivate() call on that
** a comparison to the address stored at a constant offset from that toPrivate()
** a obj->shape_->base_->clasp->flags & JSCLASS_EMULATES_UNDEFINED test

Maybe cutting off after the load, then the three comparisons, would be fast enough.  I dunno, as I said I don't ahve intuition about it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why is most of that needed?  Is it because document.all is just a random proxy?

Is there something we can do on the DOM side to make this easier?

Comment 10

[Jeff Walden [:Waldo]]

I don't think you can do anything about it.  That's to handle the case where the object being tested is a wrapper around another object; you can't test the wrapper's class flags, you have to test the wrapped object's class flags.  We could have a multiplicity of wrapper classes for falsy classes, but that seems a bit crazytalk.

I went ahead and wrote the increment to try class-pointer comparing against those three pointers, wasn't as bad as I'd thought.  Still debugging thinkos in it, no perf results for it yet.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Patch, v2.1, readier but not ready

Okay, adding slow-paths for the three proxy classes bumps v8-richards browser scores from ~6200ish to ~8800ish.  But the starting point was ~10k, so more seems to be needed, on this test at least.  (I didn't look to see what other tests need, given a sizable delta yet remains.)  Help still appreciated with this.

Comment 12

[Jan de Mooij [:jandem]]

v8-richards does many ==/!= null comparisons. It would be good to add a boolean to MCompare indicating whether OBJECT_FLAG_EMULATES_UNDEFINED is set for one of its operands. Then IsNullOr(Like)Undefined and friends don't need any temp registers or emit extra code in the common case.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Optimize comparisons

Quick-and-dirty IsNullOrLikeUndefinedAndBranch patch like I proposed in comment 12, applies on top of v2.1. unboxObject didn't exist on tip so I changed that to extractObject. This seems to fix (most of) the v8-richards regression, but other places should do something similar.

Comment 14

[Jeff Walden [:Waldo]]

Hmm, yes, I can see how that could work -- working on an updated patch with that sort of thing going in it now (will likely finish in the morning, I should stop now if I want to get to sleep at a decent hour).  It does seem worth noting that some of that delta's pessimal -- strict comparisons should not be affected by an object emulating undefined, and therefore don't need to test for undefined emulation when generating code.  I would try to fix that up, but...

This would all be clearer if there were MStrictlyEqual and MLooselyEqual, rather than just MCompare for both, and emulatesUndefined() or whatever were clearly restricted to the latter.  The algorithms are not at all the same (and become even less so with objects that can emulate undefined), so sharing code just muddies the waters.  Had I noticed and had to confront that confusion earlier, I'd have written a patch for that split first.  But I didn't, so it seems best to get this done first, then I can circle back and do that cleanup in a separate bug.  I'll try to poke you on IRC to discuss this.

(Amusingly, dvander noted when I mentioned splitting MCompare like this -- he's in favor -- that I'd done exactly the same thing to the tracer's comparison ops back in the day.  The reader may draw whatever conclusions he wishes to from this.  :-) )

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Patch, v2.9ish

Getting closer still; still needs perf testing, somehow in the last couple days my laptop no longer produces even moderately consistent performance numbers, so it looks like I'll need to test this on another system, alas.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Updated patch, more work on not testing for undefined objects when impossible

This still shows substantial perf differences on v8-richards, just posting to get it out there.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Move stub-calling to an out-of-line path; patch still buggy

This moves all calling to a stub (for the slow testing-a-proxy cases) to out-of-line code, in the hopes that doing so will recover speed lost when emitting it all inline.  It's still at least somewhat buggy; I'm seeing assertions that look type-inference-based in some cases.  Also it's against a few-weeks-old revision, although I doubt that makes too much difference in the grand scheme of things.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Possibly a fullly-working patch

Every jit-test passes with some iteration of this patch (I kept testing failures until I got it down to zero).  No guarantees about jstests, or about what happens if I throw --ion-eager at it.  No perf-testing done yet, so I don't know how much moving things out-of-line actually won yet.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Check obj->type_->flags rather than obj->shape_->base_->clasp->flags for a little speedup

I tried perf-testing the previous patch.  On a v8-richards that loops a bunch of times, I see ~1270ms timing without it, ~1420ms with.

Commenting out one of the class checks took the "with" number to ~1390ms.  I considered moving all the proxy classes into an array, then doing a range-check on the class pointer, but I think this might need another scratch register, which seems like a bad idea.

Then I had another thought: why not just check the TypeObject flags?  After a stumble forgetting that the presence of the emulates-undefined flag was insufficient to say an object emulated |undefined|, I got this patch.  With this patch I get ~1380ms.

I think there's more perf to be gained here by adding back some fallthrough possibilities that I'd removed in the name of having a clean abstraction.  At some point I suspect we'll need to eat some amount of perf change -- we're doing more work than before, and we can't expect that to be identically fast.  But I don't think we're quite there yet.  More coming.

Comment 20

[Jan de Mooij [:jandem]]

Based on the WIP patch I posted here earlier, I think all you need to unregress v8-richards is the following:

(1) LIsNullOrLikeUndefined(AndBranch) should use LDefinition::BogusTemp() if operandMightEmulateUndefined is false, just like you already have for LNotV etc.

(2) LIsNullOrLikeUndefined(AndBranch) codegen should be exactly like before if operandMightEmulateUndefined is false.

Comment 21

[Jeff Walden [:Waldo]]

Hmm.  I definitely changed code in those areas around to account for TI feedback.  I'm not sure why I didn't go all the way here, exactly.  Hmm.  Anyway, I did that and the v8-richards thing I've been running is bad at similar numbers again, which suggests this is probably good to go.  I'll post the patch for review after I finish rebasing it against current tip.

Comment 22

[Jeff Walden [:Waldo]]

Or, maybe not.  When I ran that version of the patch past jstests it pointed out some assertions I was hitting, and I think fixing those ended up regressing stuff a little.  Now it's like ~1250ms without patch to ~1295ms with it.  That's getting close enough that I should probably start comparing browser-build v8 runs -- will do that now, maybe after posting the latest version of the patch.

Comment 23

[Jeff Walden [:Waldo]]

...and it seems I'm hitting some other bugs too.  In the property-definition mega-tests with my code well out of sight.  Sigh.  The end is not quite in sight yet.

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Finally (!) good to go, I think

Okay, I think this is good to go.  Finally.  The perf difference I was seeing on the hacked-up v8-richards is gone now, so I am going to be optimistic that this represents all the perf issues, and get this up for reviewing whilst I build the browser twice to run the in-browser v8 tests.

A slightly earlier iteration of this passed try, but this is mega enough to be worth pushing a second time:

https://tbpl.mozilla.org/?tree=Try&rev=ed8c49b492ca

It is perhaps unusual to have testFoo taking two labels to jump to, rather than branchTestFoo jumping to one label and falling through to later code.  I have a delta patch atop this which will do that (which has one version usable for fallthrough and one version usable for labels-preexist cases, with a shared kernel of functionality between the two), but in my testing I'm not quite seeing a reliable perf difference between the two, so I'm kind of inclined to leave this as is unless regularity is especially desired.

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 689946 [details] [diff] [review]
Finally (!) good to go, I think

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

Looks great, mostly minor comments. Since this is a large patch I think this deserves another review though.

::: js/src/ion/CodeGenerator.cpp
@@ +172,5 @@
> +{
> +    RegisterSet temps;
> +    temps.add(scratch);
> +
> +    saveVolatile(temps);

Nit: since temps contains a single register, you can use the saveVolatile/restoreVolatile overloads that take a single register: saveVolatile(scratch).

@@ +241,5 @@
> +
> +  public:
> +    OutOfLineTestObjectWithLabels() { }
> +
> +    Label * label1() { return &label1_; }

Nit: no space after *

@@ +254,5 @@
> +
> +    // If |!(obj->type_->flags & OBJECT_FLAG_EMULATES_UNDEFINED)|, then no
> +    // object flowing through here will emulate |undefined|: the object must be
> +    // truthy.
> +    masm.loadPtr(Address(objreg, JSObject::offsetOfType()), scratch);

If the object has a lazy type it may not have the TypeObject flag set. Not sure if these objects emulating undefined can have lazy type? If so we should probably just test the Class flag.

@@ +2578,5 @@
> +    Label *doesntEmulateUndefined = ool->label2();
> +
> +    Register objreg = ToRegister(lir->input());
> +    testObjectTruthy(objreg, doesntEmulateUndefined, emulatesUndefined,
> +                     ToRegister(lir->temp()), ool);

You don't need the temp if you use "output" as scratch here.

@@ +2733,5 @@
> +    Label *ifFalsy = ool->label2();
> +
> +    Register objreg = ToRegister(lir->input());
> +    Register scratch = ToRegister(lir->temp());
> +    testObjectTruthy(objreg, ifTruthy, ifFalsy, scratch, ool);

Same here.

::: js/src/ion/LIR-Common.h
@@ +860,5 @@
> +        return getTemp(0);
> +    }
> +
> +    Label *ifTruthy() { return ifTruthy_->lir()->label(); }
> +    Label *ifFalsy() { return ifFalsy_->lir()->label(); }

Nit: for consistency format like

Label *ifFalsy() {
    return ...;
}

@@ +901,5 @@
> +        return getTemp(2);
> +    }
> +
> +    Label *ifTruthy() { return ifTruthy_->lir()->label(); }
> +    Label *ifFalsy() { return ifFalsy_->lir()->label(); }

Same here.

@@ +1203,5 @@
> +        return getTemp(1);
> +    }
> +};
> +
> +class LEmulatesUndefined : public LInstructionHelper<1, 1, 1>

Nit: this class could use a short comment.

::: js/src/ion/Lowering.cpp
@@ +360,4 @@
>      MBasicBlock *ifTrue = test->ifTrue();
>      MBasicBlock *ifFalse = test->ifFalse();
>  
> +    // String is converted to length of string in the IonBuilder phase.

Nit: "type analysis phase (see TestPolicy)."

@@ +1435,4 @@
>  {
>      MDefinition *op = ins->operand();
>  
> +    // String is converted to length of string in the IonBuilder phase.

Pre-existing nit: type analysis phase (TestPolicy).

::: js/src/ion/MIR.cpp
@@ +1123,4 @@
>              return;
>          }
>  
> +        // Swap null/undefined lhs to rhs so we can test for it only on rhs.

Nit: "only on rhs" -> "only on lhs"

::: js/src/ion/MIR.h
@@ +923,4 @@
>      AliasSet getAliasSet() const {
>          return AliasSet::None();
>      }
> +    void infer(const TypeOracle::UnaryTypes &u, JSContext *cx);

Nit: can you swap the arguments so that the cx comes first?

@@ +1400,4 @@
>      bool evaluateConstantOperands(bool *result);
>      MDefinition *foldsTo(bool useValueNumbers);
>  
> +    void infer(const TypeOracle::BinaryTypes &b, JSContext *cx);

Ditto.

@@ +3391,3 @@
>    public:
>      MNot(MDefinition *elements)
> +      : MUnaryInstruction(elements),

Pre-existing, but "elements" makes no sense. Please rename to operand/input or something.

@@ +3399,4 @@
>  
>      INSTRUCTION_HEADER(Not);
>  
> +    void infer(const TypeOracle::UnaryTypes &u, JSContext *cx);

Ditto.

::: js/src/ion/VMFunctions.h
@@ +432,4 @@
>  template<bool Equal>
>  bool StringsEqual(JSContext *cx, HandleString left, HandleString right, JSBool *res);
>  
> +uint32_t ObjectEmulatesUndefined(RawObject obj);

Nit: JSBool?

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +209,4 @@
>      }
>  
>    public:
> +    // Save and restore all volatile registers to/from the stack, excluding the

Thanks for this comment.

::: js/src/methodjit/FastOps.cpp
@@ +461,3 @@
>          /*
> +         * The other side must be null, undefined, or an object emulating
> +         * undefined; use a stub for simplicity.

Nit: "If the type of the other side is unknown, use a stub for simplicity."

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Updated for comments

As noted on IRC, I didn't change the cx-first places because infallible methods shouldn't have leading-cx argument lists.

Strangely this patch is now showing ~1275ms on v8-richards versus ~1220 without.  I'm not sure what to make of these weird swings.  :-\  Compiler code block motion maybe?  :-(

Also at this point it's probably reasonable to loop in a DOM person or two to start looking at it from that angle.  Looping in bz because he's looking at rewriting document stuff now or soonish...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 690971 [details] [diff] [review]
Updated for comments

r=me on the classinfo bits.  Looks good!

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 690971 [details] [diff] [review]
Updated for comments

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

Nice work! r=me with nits below addressed.

::: js/src/ion/CodeGenerator.cpp
@@ +2761,5 @@
> +    Label *ifTruthy = ool->label1();
> +    Label *ifFalsy = ool->label2();
> +
> +    Register objreg = ToRegister(lir->input());
> +    Register output = ToRegister(lir->getDef(0));

Nit: s/getDef(0)/output()

@@ +2806,2 @@
>      Label join;
> +    Register output = ToRegister(lir->getDef(0));

Nit: s/getDef(0)/output()

::: js/src/jit-test/tests/truthiness/equal-null.js
@@ +1,3 @@
> +function f(v, value)
> +{
> +  var b = v == null;

Can you add a version of some of these tests (or a second function in the same file) where v is always an object? E.g. don't pass null, undefined or another primitive to it, to test the paths in the compiler where we have an Object instead of Value. We should also have some jit-tests for typeof, NotO/NotV and TestOAndBranch.

Comment 29

[Jeff Walden [:Waldo]]

Attachment: With those updates, and moar tests

Still need to do more perf examination, if the 1220/1275 bit noted in the previous comments matter.  :-\

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Even moar tests, because jandem isn't easily satisfied :-P

Comment 31

[Ed Morley [:emorley]]

Backed out for assertions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d54961cd14

Comment 32

[Jeff Walden [:Waldo]]

The assertions are because script() produces an Unrooted, and ->infer() when it can call hasObjectFlags() can trigger GC.  I just assigned oracle->{unary,Binary}Types(...) to a temporary and used that, so that the Unrooted would be destroyed before the ->infer might need to GC.

There were also a couple always-methodjit failures in truthiness/not.js, depending on shell flags.  The problem there was that I'd changed the known-type path in jsop_not() in the methodjit, but not the unknown-type path.  evilpies noted that I just needed to remove the if-object inline path there and supplied a patch, which with a typo fix from me worked like a charm.

Those changes made, and given this is kind of complex enough that I'm leery of eyeballing that fix, I pushed this off to try for a conclusive report:

https://tbpl.mozilla.org/?tree=Try&rev=1f5c9c59cca8

On first landing AWFY showed a spike for Firefox (JM+TI), which is probably because I didn't go to heroics to optimize methodjit for this change.  It didn't show a spike for JM+TI+Ion, tho, which is what matters, so assuming this passes try, this should be good.

Comment 33

[Jeff Walden [:Waldo]]

Try results were broadly green, and green particularly on the oranges seen on the first m-i push (which itself, aside from those oranges, was broadly green), so I repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab307f02af0c

Comment 34

[Ed Morley [:emorley]]

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

Comment 35

[Jeff Walden [:Waldo]]

I mentioned the new class flag on:

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JSClass.flags

This needs to be added to release notes for SpiderMonkey 20, which to the best of my knowledge haven't been started yet, so I'm keeping this open til then.

Comment 36

[Jeff Walden [:Waldo]]

Attachment: Patch that makes testObjectTruthy/testValueTruthy into branch*Truthy, with fallthrough

I wrote this initially thinking that it might be needed to speed things up, by having fallthrough rather than jumps to the next thing to execute, in some cases.  It now seems not to have been necessary for perf.

It might still be valuable as a cleanup, tho -- testValueTruthy and testObjectTruthy are the only methods I'm aware of where control flow past a call to them doesn't continue in source order.  At this point I'm not sure I feel strongly about it, tho.

I'd like to not have to carry this patch around, so I'm uploading the last possibly-working version I have of it here.  If we ever decide we want more traditional control flow, it'll be ready for use, or examination, or whatever.

Comment 37

[Jeff Walden [:Waldo]]

On considering that this flag shouldn't affect strict equality, I went back and added corresponding strict-equality and strict-inequality tests for all this, that basically copy the loose-equality tests and flip answers as appropriate.  I also added some tests that loose-equality object-object comparisons ignore this flag and only compare objects emulating undefined equal if they're the same object.  It's all tests, and mostly copying existing tests, and it passes a shell build with --tbpl, so I pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/62c52b199c9a

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/62c52b199c9a