728411 - Move object implementation details out of JSObject into ObjectImpl, Elements, and Properties classes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

jsobj.h and JSObject are kind of a mess of complexity, with some public methods, some private methods, and not a whole lot of distinction between the two.  It's a very fat interface.  I'd find it helpful for the property split if JSObject were just a veneer that acted upon a lower-level interface, an ObjectImpl superclass if you will.  That class would act upon the type, shape, slots, and elements directly.  The complexity of those structures should also be moved into separate headers.

It's also possible this might make it easier to have a compile-time switch to allow gradual implementation of a new property-storage mechanism, with the final switchover controlled by #ifdef.  This might or might not be feasible.  But anything that makes it easier to make a controlled step, that doesn't require landing and enabling everything all at once, seems highly desirable.  So I think this is worth doing for that reason as well.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Move ObjectElements and a little bit of implementation related to it into a separate header

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Add ObjectImpl as an intermediate class between JSObject and Cell

There's a lot of stuff that relies on other members being directly in that class, so things will have to move from JSObject to ObjectImpl incrementally.  The alternative is moving everything at once, which seems a bit harder to review.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 598378 [details] [diff] [review]
Move ObjectElements and a little bit of implementation related to it into a separate header

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

I think that ObjectElements should be in the same header and files as ObjectImpl.  ObjectElements is a very thin structure which only makes sense in the context of the object that owns it, and the comments and code for all of these should be in the same place.  The model here is Stack.h, which has a lot of separate structures which are all interrelated, and would be much harder to understand if those structures were in separate headers.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: New files, take two

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Start moving methods over, a bunch of simple methods at first

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Move some sizing bits

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Make JSObject::reportReadOnly a static non-JSObject method

Turns out this method doesn't even use |this|, not sure why it existed on JSObject in the first place, probably historical reasons...

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Move a bunch of element bits over

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Barrier bits

Comment 10

[Jeff Walden [:Waldo]]

Attachment: More elements stuff

Somewhere in this stream of patches I start to copy over is* methods without copying over the corresponding set* methods.  This could possibly be avoided, but it'd be a lot of work.  The problem is the set* methods depend on setFlag, which is non-trivially intertwined with a bunch of JSObject stuff as I remember.  It'll move over at some point, just it's a bit of trouble to do it now, exactly.

Comment 11

[Brian Hackett [Laid off!]]

Comment on attachment 599354 [details] [diff] [review]
New files, take two

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

::: js/src/vm/ObjectImpl.cpp
@@ +21,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2012
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Jeff Walden <[e-mail]> (original author)

This is kind of petty, but can you leave the contributors section blank, like it is in jsobj*?  Seeing contributor lists in the source that are woefully incomplete has bugged me for a while, and it would be nice if they either (a) reflected everyone who has made significant contributions to the code in the file, or (b) are left blank.  The 'Spidermonkey elements' bit above is wrong, too.  Ditto for the other new files.

Comment 12

[Terrence Cole [:terrence]]

Shouldn't you be using the new MPL2 license block anyway?  http://www.mozilla.org/MPL/headers/

Comment 13

[Jeff Walden [:Waldo]]

Um.  That page might say so.  I guess I'm going to have to vigorously protest it, or at least request that bug 717196 get prioritized.  That's the reason I haven't been using it -- that I very much don't want to lose MXR functionality just for a new header.

Comment 14

[Brian Hackett [Laid off!]]

Comment on attachment 599355 [details] [diff] [review]
Start moving methods over, a bunch of simple methods at first

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

::: js/src/jsobj.h
@@ -503,4 @@
>      inline bool isSystem() const;
>      inline bool setSystem(JSContext *cx);
>  
> -    inline bool hasSpecialEquality() const;

I think this should stay in JSObject.  My understanding of the ObjectImpl vs. JSObject split is that the former contains the minimum of methods to interact with the object's layout, and the latter has convenience methods that wrap the former and are not related to the object implementation.  hasSpecialEquality seems to fit the latter better.

(Aside, just did some grepping.  Is hasSpecialEquality used at all anymore?)

Comment 15

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81fed0de0cb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a67912564bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/19f1e4c45501
https://hg.mozilla.org/integration/mozilla-inbound/rev/02af20a780d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fe202b98d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/64dead6fa841
https://hg.mozilla.org/integration/mozilla-inbound/rev/b150d708bb98

More to come here, further patchwork is still happening in my mq...

Comment 16

[Jeff Walden [:Waldo]]

Regarding comments:

I used the MPL2 header, and I added grist to the bug 717196 mill to hopefully get that issue resolved sooner.

hasSpecialEquality is I think more of an implementation detail than something we will ultimately want to expose in JSObject.  Generally users shouldn't be asking the question, and the only things that need to know such intimate details are JITs, which of course are going to be ObjectImpl-aware.  But I could be wrong about how this split should play out, so I left it where it was for now.

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/81fed0de0cb4
https://hg.mozilla.org/mozilla-central/rev/0a67912564bb
https://hg.mozilla.org/mozilla-central/rev/19f1e4c45501
https://hg.mozilla.org/mozilla-central/rev/02af20a780d6
https://hg.mozilla.org/mozilla-central/rev/b7fe202b98d3
https://hg.mozilla.org/mozilla-central/rev/64dead6fa841
https://hg.mozilla.org/mozilla-central/rev/b150d708bb98

Comment 18

[Ed Morley [:emorley]]

Either this or one of the other bugs in the same push caused:

Regression :( Trace Malloc MaxHeap increase 2.11% on CentOS release 5 (Final) Mozilla-Inbound
---------------------------------------------------------------------------------------------
    Previous: avg 26103806.667 stddev 15352.186 of 30 runs up to revision ede36f262612
    New     : avg 26653960.000 stddev 3571.834 of 5 runs since revision b150d708bb98
    Change  : +550153.333 (2.11% / z=35.836)
    Graph   : http://mzl.la/zcFtKt 

and 

Regression :( Trace Malloc MaxHeap increase 2.32% on WINNT 5.2 Mozilla-Inbound
------------------------------------------------------------------------------
    Previous: avg 24745420.000 stddev 30508.545 of 30 runs up to revision ede36f262612
    New     : avg 25318560.000 stddev 43905.159 of 5 runs since revision b150d708bb98
    Change  : +573140.000 (2.32% / z=18.786)
    Graph   : http://mzl.la/xMpaaj 


https://groups.google.com/d/msg/mozilla.dev.tree-management/O57mK7haIQk/nAHeyQWXancJ

Comment 19

[Jeff Walden [:Waldo]]

There shouldn't have been any heap-usage change from the patches in this particular bug -- all it did was insert a class in a class hierarchy (one without vtables, too) and move some methods and fields among the classes in it.  I suppose it's not impossible it's one of the other patches there, but I'm still a bit surprised about this...

Comment 20

[Ed Morley [:emorley]]

I just picked one of the bugs in the push at random to post the above in, I didn't have a chance to look at the diffs. The regression is quite noticeable on the graphs - can I leave you to narrow down which of your changesets in that push caused it? :-)

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Move more slot range details into ObjectImpl

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Move various other slots-access methods

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Move shape consistency checking code

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Move a couple more slot methods

Comment 25

[Brian Hackett [Laid off!]]

Comment on attachment 606761 [details] [diff] [review]
Move more slot range details into ObjectImpl

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

::: js/src/jsobj.h
@@ +1133,2 @@
>      inline bool isDate() const;
> +    using js::ObjectImpl::isDenseArray;

What does this code for? (C++....)  isArray etc. are public members of ObjectImpl and this code should be a no-op, so can you just update the comment to point readers at the ObjectImpl section instead?  I'm also hoping that once the elements stuff is fixed that ObjectImpl will be totally class agnostic and won't need any isClass methods.

@@ +1147,4 @@
>      inline bool isRegExpStatics() const;
>      inline bool isScope() const;
>      inline bool isScript() const;
> +    using js::ObjectImpl::isSlowArray;

Ditto.

Comment 26

[Jeff Walden [:Waldo]]

(In reply to Brian Hackett (:bhackett) from comment #25)
> Comment on attachment 606761 [details] [diff] [review]
> What does this code for?

Not much, I guess.  I think maybe those methods were only protected, so the using was necessary, then I made them public at some point and they became unnecessary.

> I'm also hoping that once the elements stuff is fixed that ObjectImpl will be
> totally class agnostic and won't need any isClass methods.

Yeah, I think that's an end goal here.

Comment 27

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a83bc932f2f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dd38e12421
https://hg.mozilla.org/integration/mozilla-inbound/rev/791d47cc688b
https://hg.mozilla.org/integration/mozilla-inbound/rev/76ea55870050

Still more coming here, although I'm starting to work on the element stuff now and will probably only return to this intermittently as I need things here.

Comment 28

[Jeff Walden [:Waldo]]

Plus this, because MSVC is dumb:

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

Comment 29

[Jeff Walden [:Waldo]]

Attachment: Move private data details into ObjectImpl

I found I needed this for ArrayBuffer, which currently stores its delegate object (that holds the array buffer's actual properties) as a private.  I'm pretty sure that'll change so that ArrayBuffer can just be a regular old object, but it seems best to leave that to later after more of the new representation gets fleshed out.

Comment 30

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a83bc932f2f0
https://hg.mozilla.org/mozilla-central/rev/d8dd38e12421
https://hg.mozilla.org/mozilla-central/rev/791d47cc688b
https://hg.mozilla.org/mozilla-central/rev/76ea55870050

Comment 31

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2be844125c05 for comment 29's move-private-stuff-to-ObjectImpl.  Still keeping open...

Comment 32

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2be844125c05

Comment 33

[Terrence Cole [:terrence]]

I'm guessing we can close this now.