408416 - JSObjectOps needs a make-over (JSObject layout too)

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Brendan Eich [:brendan]]

JSObjectOps was a "poor man's vtable" introduced on top of existing layering back in the late 90s, by Scott Furman and myself for Scott's LiveConnect (IIRC this was the third incarnation), then used by XPConnect as well. I extended it for E4X with JSXMLObjectOps.

As Igor has pointed out in bug 355257, we have lots of abstraction leaks, places where object ops pointer, a method pointer, a class pointer, or some other test is done to distinguish, e.g. native from foreign object ops, or native from E4X. Some of these leaks were always there -- fur and I got lazy, or we were in a hurry back in the Netscape days. I wouldn't even call JSObjectOps much of an abstraction, but it has proven useful for *varying* behavior and *specializing* property lookup, e.g.

Without duplicating or necessarily pre-empting bug 355257 (not sure, marking this bug blocking that one for now), I though I would file this bug to take a survey of exactly how all hooks in JSObjectOps and JSXMLObjectOps are used, as well as places where the abstraction leaks and if tests are used, and try to "make over" JSObjectOps, in order to (not in priority order):

1. Clean up code that violates the abstractions, such as they are.
2. Avoid unnecessary and unwanted abstraction points.
3. Speed up and simplify the code involved.

I do not think we should wait for Mozilla 2 to break JSObjectOps API, it has always been a moving target compared to JSClass (this may hurt some embedders, but they aren't exactly being helped by the current situation).

Comments welcome.

/be

Comment 1

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

Yeah, it was possible to do reflectconnect on the raw JSClass, but not to do it well. :)

I support all 3 of these endeavours, and also doing it ahead of Mozilla 2.  We're getting late in the beta cycle to avoid being effectively last-minute for extensions that use these ops, but we can decide the landing schedule when we have patches about which to decide.

Comment 2

[Brendan Eich [:brendan]]

Shaver: do you know of any extensions that include binary components that use JSObjectOps? I do not. I'd be surprised. Will ask on m.d.t.js-engine when I get the chance.

/be

Comment 3

[Brendan Eich [:brendan]]

This looks important for bug 322889.

/be

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)

The JSAPI doesn't seem to expose the right stuff for applications to make custom JSObjectOps.  As it stands, jsapi.h gives you neither JSObjectMap's definition nor js_ObjectOps, so it's hard to populate JSObjectOps.newObjectMap (whether with a custom implementation or the default one).

You can get js_ObjectOps by getting a standard JSClass and call clasp->getObjectMap.

I've been documenting these (for the benefit of ActionMonkey object model work), but of course don't let that stop you...
  http://developer.mozilla.org/en/docs/JSObjectOps.newObjectMap
  http://developer.mozilla.org/en/docs/JSObjectOps.lookupProperty

Comment 5

[Brendan Eich [:brendan]]

Thanks, good to know about those docs -- we'll break them ;-).

Goal is to put ops first in JSObject, followed by dslots, followed by fslots up to a class-specific limit, so you can allocate larger native objects (e.g. for shaver's array patch0. The map pointer moves into a fixed slot for those objects that need it (JSScope * for native objects).

/be

Comment 6

[Mike Moening]

I noticed this note in the developer docs: 

"Warning: Creating and using custom JSObjectOps is a bit risky, as is not considered a stable API. See bug 408416."

Exactly what risks should I be aware of as a muli-threaded developer (THREADSAFE).

Comment 7

[Brendan Eich [:brendan]]

Mike, that doc uses "not stable" to mean "likely to change incompatibly", not "buggy or crashy." This bug indeed proposes incompatible changes to JSObjectOps, but they'll only go in once "stable" in the not-buggy/crashy sense.

API stability for SpiderMonkey means frozen APIs, but JSObjectOps had a lower melting point and never reached the same state of hardness as the rest of jsapi.h.

/be

Comment 8

[Brendan Eich [:brendan]]

Prefer not to rush this, but not tarry either (so Firefox 3.1 could be a target if that comes about when it might ;-).

/be

Comment 9

[Jason Orendorff [:jorendorff]]

<jorendorff> brendan: do non-native objects necessarily have titles?
  they don't, right?
<brendan> no, they don't
<brendan> jorendorff: non-native obj->map may be neither scope nor
  have-a-title in the poor-man's-mixin after map sense
<jorendorff> brendan: isn't there a catch-22 there?
<jorendorff> I can only find out if an object is native by reading map->ops
<jorendorff> which, if the object is being used in another thread, may have
  been destroyed
<brendan> yes, catch-22
<jorendorff> ...but I can't lock the object without checking...
<brendan> jorendorff: how about we require native only for js_IsCallable
<brendan> it's inside the API firewall
<brendan> will fix all this in the object/JS_THREADSAFE make-over
<jorendorff> well ... I think the catch-22 is more pervasive than this though
<brendan> probably -- i mentioned earlier today that this might not be the
  first unlocked OBJ_IS_NATIVE ;-)
<jorendorff> simplest case, if we just want to OBJ_GET_PROPERTY(cx, obj, id, vp)
<jorendorff> that's doing an unlocked read of obj->map->ops
<jorendorff> in order to call the function that does the locking
<brendan> right -- ops should be in obj
<brendan> not in obj->map
<brendan> this is the primal sin
<jorendorff> yeah
<brendan> the bad case is an MT object
<jorendorff> that sounds true to me
<brendan> not a problem in our world
<brendan> maybe MikeM
<jorendorff> that's true too
<brendan> but worse, it has to be losing its proto
<brendan> preemption in a small window
<brendan> can happen, i've debugged kernel bugs like it
<brendan> murphy was an optimist
<brendan> but we can dodge this bullet a bit longer, like keanu bending
  backwards in the matrix

Comment 10

[Jason Orendorff [:jorendorff]]

<brendan> make sure you get the keanu bit ;-)
<sayrer_> http://dashes.com/anil/2009/03/whoa-unto-thee.html

Comment 11

[Brendan Eich [:brendan]]

To have the old scope go away requires the prototype from which it was shared to be GC'ed, or to have its __proto__ set (which requires a GC). This cannot happen while a request is active (the one being preempted after it loads obj->map but before it loads ...->ops). So the request model saves the day again, Keanu lives.

Still want this make-over, of course.

/be

Comment 12

[Brendan Eich [:brendan]]

Ah, but the request model can't help if obj has already lots its prototype, so it holds the last ref-count on the formerly-shared scope. But scope->object has been set to null (by js_FinalizeObject => js_DropObjectMap). The missing step I had ass-u-med, which should be patched to close this hole, is in js_GetMutableScope:

diff --git a/js/src/jsscope.cpp b/js/src/jsscope.cpp
--- a/js/src/jsscope.cpp
+++ b/js/src/jsscope.cpp
@@ -65,16 +65,20 @@ js_GetMutableScope(JSContext *cx, JSObje
     JSScope *scope, *newscope;
     JSClass *clasp;
     uint32 freeslot;
 
     scope = OBJ_SCOPE(obj);
     JS_ASSERT(JS_IS_SCOPE_LOCKED(cx, scope));
     if (scope->object == obj)
         return scope;
+    if (!scope->object) {
+        scope->object = obj;
+        return scope;
+    }
     newscope = js_NewScope(cx, 0, scope->map.ops, LOCKED_OBJ_GET_CLASS(obj),
                            obj);
     if (!newscope)
         return NULL;
     JS_LOCK_SCOPE(cx, newscope);
     obj->map = js_HoldObjectMap(cx, &newscope->map);
     JS_ASSERT(newscope->map.freeslot == JSSLOT_FREE(STOBJ_GET_CLASS(obj)));
     clasp = STOBJ_GET_CLASS(obj);

I'll get this into a separate bug. We should fix it in 1.9.1.

/be

Comment 13

[Brendan Eich [:brendan]]

Ah^2, to get obj to have a map that is not its own, which is a scope whose object member is null, can't happen. The OBJ_SCOPE(obj)->object == NULL condition implies that the formerly-scope-owning object has been finalized. But it can't be garbage if obj->fslots[JSSLOT_PROTO] references it. Therefore obj.__proto__ = ... must have been run, but (see js_GC) that will give obj its own scope if ... is null or non-native or different-class, else a non-owning share in ...'s scope.

The patch directly in comment 12 was troubling me also because STOBJ_NSLOTS(obj) vs. scope->map.freeslot -- the two could be unrelated due to obj having reserved slots set on it while it did not own its scope, and scope's object (GC'ed now under the hypothesis) having potentially many slot-ful props as well as (possibly different) reserved slots.

But the hypothesis is not valid. The __proto__-setting path avoids leaving obj with an orphaned scope.

Note that js_LookupPropertyWithFlags will skip over an orphaned scope, dodging the slotful JSScopeProperty indexes off the end of any (or a null!) object ref.

That Keanu can dodge anything!

No new bug, probably just tighter assertions in the patch for this bug.

/be

Comment 14

[Jason Orendorff [:jorendorff]]

What exactly do we want to do in this bug?

JSObject layout stuff I would like to see:

- ops belongs on obj, not map
- shape belongs on obj, not map, for cache-ready object types
- obj should be something we can easily subclass, as we do with JSFunction

JSObjectOps stuff I would like to see:

- New ops "getPropertyForCache", etc. that populate the property cache.  The interpreter/recorder would only call these on a cache miss.

- New ops for stuff like "hasInstance", "hasProperty", "hasOwnProperty", so that (a) lookupProperty isn't quite so overloaded and thus doesn't have to use cx->resolveFlags and bytecode inspection; (b) we can do everything we need to do to an object via ops, so objects are wrappable and ops are easily stackable.

Note that the current machinery avoids recursing on the C stack by iterating instead when the prototype _IS_NATIVE.  If we plan to wrap and stack freely, we'll want to avoid recursion in a less hackish way, e.g. by designing the ops to do tail calls or by making some ops refer only to "own" properties.

JSAPI changes I'd like to see:

- Stop exposing JSObjectOps via jsapi.h.

- But add a stable API (to replace JS_InitClass) for creating a constructor from a static class-description.  This would create a new JSObjectOps behind the scenes.

- Add an API for creating objects from constructors, as though with `new Banana(a, b, c)`.

- Add a stable API for wrapping objects (and maybe for hacking objects via stackable JSObjectOps).

- Kill resolve flags, of course.  Deprecate JS_LookupPropertyWithFlags.

That's enough for several bugs, I hope.  The only piece I need today is the hardest one: "[make it so] we can do everything we need to do to an object via ops, so objects are wrappable and ops are easily stackable".

Comment 15

[Brendan Eich [:brendan]]

The wiki is better for the fuzzy front end (mistake #19) stuff. We tend to avoid wasting time in bugzilla on fuzzy front end stuff by patching what's not fuzzy, but look where it's got us!

I have more thoughts about shared JSScopes and JS_THREADSAFE elimination, I will save 'em for the wiki.

In this bug, listing all the OBJ_IS_NATIVE tests above the existing JSObjectOps layer would be non-fuzzy and helpful.

/be

Comment 16

[Brendan Eich [:brendan]]

Jason started https://wiki.mozilla.org/JSObject_Makeover and I edited a bit.

/be

Comment 17

[Jason Orendorff [:jorendorff]]

Status report:

JSObjectOps has a lot of users--2 implementations in XPConnect and 3 in LiveConnect, 75 places in the engine itself where {OBJ,MAP}_IS_NATIVE are used, not to mention places where ops are actually called. I *do* want to work piecemeal--in regular-sized patches, not huge ones--so I'll know when I break something somewhere.

For stackable ops to work, there must be a finite, well-understood set of leaks in the abstraction (ideally none).  We have some that seem like they could be easily fixed.  For example, JS_LookupPropertyWithFlagsById has this code:

    ok = OBJ_IS_NATIVE(obj)
         ? js_LookupPropertyWithFlags(cx, obj, id, flags, objp, &prop) >= 0
         : OBJ_LOOKUP_PROPERTY(cx, obj, id, objp, &prop);

This code is bad for stackable ops. Either the stacked object will appear to be native, in which case the ops are bypassed and js_LookupPropertyWithFlags is called directly; or it will appear non-native, and then the flags are lost. There are similar places in js_LookupPropertyWithFlags itself. The easy fix is to change the lookupProperty op to have the same signature as js_LookupPropertyWithFlags.

defineProperty needs similar treatment.

When starting small like this, the risk is that you're not setting out on a path you can walk in a reasonable amount of time.  I admit I am a little overwhelmed (today, anyway).  But I don't need everything at once, just stackable ops, so there's a nice finite-sized target to shoot for.

See you in two weeks.

Comment 18

[Brendan Eich [:brendan]]

Seems worth considering bug 437325 as a dependency of this bug. Let me know if that's not helpful.

/be

Comment 19

[Igor Bukanov]

(In reply to bug 491126 comment #20)
> > I think for 1.9.2 we should consider merging JSObjectOps and JSClass into one
> > entity. Those members can still live in the JSObjectOps struct to emphasis the
> > non-public-and-free-to-change status.
> 
> We have some name conflicts (getProperty, etc.). The origins and design targets
> differ too, JSClass was lower-level with common hashtable machinery above it in
> the original design; JSObjectOps was the higher-level "switch" by which the
> Java to JS bridge could bypass the hashtable stuff.

Which suggests to move JSObjectOps methods that native objects can overwrite while still staying native (like clear, for example) into JSClass. Those new fields should be marked as experimental so the embedding can not expect their stability. The rest of JSObjectOps would thus represent the true higher-level "switch" and the object should store the union of (JSClass, JSObjectOps) in place of the current classword field.

That is, the idea is to keep JSClass only for native objects. Non native would not have such notion and would use the abstract JSObjectOps.

Comment 20

[Jason Orendorff [:jorendorff]]

The reasoning in comment 13 has a hole, as illustrated by bug 503155 comment 8.

I don't think the patch in comment 12 is correct either.

Only human.  :-|

Comment 21

[Brendan Eich [:brendan]]

The patch-let in comment 12 is definitely obsolete, but there's no checkbox to so mark it.

Don't root for Agent Smith yet, though. With bug 503080 fixed the revo^H^H^H^H make-over can commence!

/be

Comment 22

[Brendan Eich [:brendan]]

Andreas suggested replacing JSObjectOps with mighty if statements testing obj->isProxy(). Aside from that being two class tests and so not as fast as it should be, we'd probably gain inlining and branch-prediction (with PGO or proven-beneficial JS_UNLIKELY) perf. We can do this in steps:

1. Make dense array-ness universal for all non-proxy objects.

2. Convert E4X and anything else that should be non-native but isn't to Proxies.

3. Convert any truly non-native JSObjectOps impls to Proxies.

4. Get rid of JSObjectOps.

Comments?

/be

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information