610437 - clasp->ext.equality should continue to be available for embedders

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Franck]

Comparing two object instances with the == operator is more convenient than using a .equals() function.

Comment 1

[David Mandelin [:dmandelin]]

Keep in mind that having this feature makes it harder to make '==' run fast.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> Keep in mind that having this feature makes it harder to make '==' run fast.

E4X adds an extra check on == path in any case. So presence of ext.equality should not slow down us as long as we have to support E4X.

Comment 3

[Andreas Gal :gal]

We want to move E4X out of the main VM into some emulation mode. E4X is definitely not a good reason to keep the equality hook.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

For E4X specifically, we can detect when E4X stuffs enter a given compartment, and recompile with deoptimized code.

Comment 5

[Andreas Gal :gal]

Actually we don't support wrapping e4x objects across compartments, so just setting a flag on the compartment when an e4x object is instantiated will do the trick.

Comment 6

[Brendan Eich [:brendan]]

We are looking into value types ("universal proxies"), see http://wiki.ecmascript.org/doku.php?id=strawman:value_types and http://wiki.ecmascript.org/doku.php?id=harmony:proxies (also noted in the latter: http://slang.soe.ucsc.edu/cormac/proxy.pdf).

E4X's runtime will be reimplemented on proxies in bug 485791 -- this will find gaps between E4X and Proxy semantics (not call vs. get, though).

Odds are hard to place but I think we'll have operators in some form in a future release, prototyping proposals before Ecma TC39 for value types and an ECMA-357 successor and final resting place.

In ECMA-262 (JS itself, no extensions), == involves conversions if both operands are not object, so it is already hard to make == run fast in edge cases. Those edge case deoptimization paths must exist already. For anything new like value types, the first place they'd arise would be in those corners, not in any fast path (which would be incompatible or just unwarranted).

So we should not protest too much about this bug. Embedders want what we want. It's a signal from the design of the language (and its gaps); it's not noise.

/be

Comment 7

[Brendan Eich [:brendan]]

As far as embedding APIs go, we have three options:

1. No public API, you bootleg your own, at your own risk. Future SpiderMonkey releases may break you freely.

2. A public API, we'll try to avoid changing it as usual, it's not frozen but it is public and other than fixing design flaws and bugs, both implemntor and all the embedders win if it is stable.

We had 2, we took it away. If we do anything like comment 6, it will be later, after Mozilla 2 / Firefox 4 / JS1.8.5. So we shouldn't try for a "new 2". That leaves option 1 for now.

/be

Comment 8

[Brendan Eich [:brendan]]

Oops, did I saw three options? I think I meant only two. I had thought we could make a friend API to avoid exporting too many header files, but we already export jsvalue.h. So option 1, pro tempore.

/be

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: proposed fix

I think this patches fixes the problem that the reporter was having, which is that equality ops stopped working in the method jit. That was caused by bug 599214, where we added a flag for equality ops in order to save a load.

There's also have the issue that equality ops were removed from jsapi.h. I think this happened a while ago, in bug 571789. This patch doesn't address that.

Comment 10

[Luke Wagner [:luke]]

Three requests:
1. could you factor out the "if (clasp.ext->equality) ..." check, perhaps into JSObject::syncEqualityFlag();
2. there seem to be a couple ways for an embedders' clasp's equality to skip the flag check: js_InitClass (calling NewObject), JS_ConstructObject, JS_NewGlobalObject.
3. Could you add an assert

    JSObject::assertEqualityFlagSynced() {
      JS_ASSERT_IF(clasp.ext->equality, obj->flags & JSObject::HAS_EQUALITY);
    }

and call that from JSOP_EQUALITY and the analogous pinch point in the mjit

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: updated patch

Yes, that's much better. I fixed these issues.

I also noticed that when compiling without JS_HAS_XML_SUPPORT, the interpreter completely ignores ext->equality. Based on the direction we seem to be going in this bug, I think we should fix this. I folded that into the patch as well.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 489719 [details] [diff] [review]
updated patch

Thanks!

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/b45a434dbbdd

It seems like there are two remaining issues:
1. We still don't expose clasp->ext.equality through JSAPI.
2. We could be optimizing object equality a little more aggressively, which would eliminate a branch in JM and TM. This would require the compartment tainting changes.

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b45a434dbbdd