588774 - Make subclasses of JSObject

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The interface of JSObject is absurdly fat. For example, it has 11 methods and 5 constants that are only meaningful for Arguments objects; and more for Date, Function, RegExp, and so on. Even XML. Inline member functions of JSObject are defined in about 11 different header files now.

This is outrageously silly. Those should be subclasses.

Comment 1

[Brendan Eich [:brendan]]

It's true. Nick did help encapsulate raw slots usage (bug 565845 still lives but he resolved a bunch, it was good work), which alas made JSObject a lot wider. It was a calculated trade-off and not the final word. We talked then about getting to the subtypes promised land. First fatten JSObject, then split it up.

Waldo raised the desire for typed members instead of tagged fslots in bug 429507 comment 15. We all feel it.

There's some tension with the variable fslots based on a allocation-site cache prediction work that is imminent after bug 558451 is landed: bhackett owns that in bug 584917 (see also earlier bug 547327).

It seems to me we could have subtypes and variable length fixed slots, but the latter would have to come after members added by the former, or else we'd have to pin the number of fixed slots per subtype.

The variable length fslots wins are mainly important for Object and Array (possibly Function too, needs study), so the conflict may be less than it could be, in practice.

/be

Comment 2

[Brian Hackett [Laid off!]]

Is the desire for a better interface, or subtypes that extend the data fields of JSObject?  Both should be compatible with 584917, but the former is way easier --- just make subtypes of JSObject which define new member functions but no new fields.  I'm not sure what the latter is buying; most objects (except dates) have few reserved slots that hold non-values, so this would sometimes save a few bytes while probably increasing complexity in both the GC and 584917 quite a bit.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> Is the desire for a better interface, or subtypes that extend the data fields
> of JSObject?

Better interface is plenty for one bug.

Comment 4

[Luke Wagner [:luke]]

Is the basic plan still to have a pair

  bool JSObject::isX()
  X *JSObject::asX()

for X = { RegExp, Args, DenseArray, ... } ?

Comment 5

[Jason Orendorff [:jorendorff]]

Comment 4 would be fine with me.

We could use templates instead.

  template <class T>
  X *is() { return clasp == T::classSingleton(); }

  template <class T>
  X *to() { JS_ASSERT(is<T>()); return (T *) this; }

  template <class T>
  X *as() { return is<T>() ? to<T>() : NULL; } 

but maybe obj->is<FunctionObject>() is just too ugly compared to obj->isFunction().

This could be taken a little further, so that:

  if (FunctionObject *funobj = obj->dcast()) ...
  if (FunctionObject *funobj = value.dcast()) ...

a la do_QueryInterface. template<class T> operator T*() seems to work in MSVC. Not that that makes it a good idea.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #0)
> 
> This is outrageously silly. Those should be subclasses.

I'm glad you noticed.  See bug 566789, which starts fixing this.  Its patch has r+ from Waldo, unofficial r+ from Luke, and (IIRC) tacit approval from Brendan.  The sub-classes don't have extra data fields.

I also have numerous other bugs open (eg. bug 565845) to fully encapsulate fslots and dslots (several which also have r+) which would make things like type-specific slots simpler to implement.  But all this mere clean-up work has been languishing while big perf-critical bugs like fatvals and Brendan's scope removal have been in progress.

Comment 7

[Paul Wright]

Is this bug still active / valid?

Comment 8

[Jeff Walden [:Waldo]]

Yes.  I've been carrying a patch to add NumberObject for months now (waiting on other patches in my mq to land).  Others will come over time.  It's not a huge priority, tho, so it only happens as people make the effort.

Comment 9

[Paul Wright]

Since OBJSHRINK is complete, back to comment 7....

Comment 10

[Jeff Walden [:Waldo]]

RegExp has its own subclass now; ditto for Boolean, Number, and String.  I don't believe Date does yet, but I could be mistaken about that.  Call objects have a hierarchy of subclasses deriving from the newly-named ScopeObject class.  XML objects don't, and I suspect probably won't given the low value and moderate cost to making it happen there.

Basically, this has been happening piecemeal in other bugs for quite some time.  It's probably not necessary to have a bug specifically devoted to it.  If this were my bug I'd just close it and let the separate-bug approach keep doing its thing, but that's enough a matter of personal hygiene I'm not going to do it to a bug that's not mine, that I didn't file.

Comment 11

[Paul Wright]

jorendorff, would you respond to comment 10 and/or take action?

Comment 12

[Paul Wright]

This seems to be a dead bug.. jorendorff?

Comment 13

[Jason Orendorff [:jorendorff]]

==> RESO WORKSFORME.

Comment 14

[Nicholas Nethercote [inactive]]

FWIW, I've just about finished converting all of these.  See bug 884124 and bug 887558.