Closed Bug 610437 Opened 9 years ago Closed 9 years ago

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


(Core :: JavaScript Engine, defect)

Windows XP
Not set





(Reporter: soubok, Unassigned)



(1 file, 1 obsolete file)

Comparing two object instances with the == operator is more convenient than using a .equals() function.
Keep in mind that having this feature makes it harder to make '==' run fast.
(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.
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.
For E4X specifically, we can detect when E4X stuffs enter a given compartment, and recompile with deoptimized code.
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.
We are looking into value types ("universal proxies"), see and (also noted in the latter:

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.

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.

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.

Attached patch proposed fix (obsolete) — Splinter Review
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.
Attachment #489561 - Flags: review?(lw)
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
Attached patch updated patchSplinter Review
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.
Attachment #489561 - Attachment is obsolete: true
Attachment #489719 - Flags: review?(lw)
Attachment #489561 - Flags: review?(lw)
Comment on attachment 489719 [details] [diff] [review]
updated patch

Attachment #489719 - Flags: review?(lw) → review+

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.
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.