648647 - Eliminate JSObjectMap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Aside from the sharedNonNative singleton, there aren't any instances of JSObjectMap. It seems to exist to appease the header file gods.

If we eliminate it, we get rid of a union inside JSObject. For the write barriers work, I'd like to wrap the |lastProp| field in a container class, and the union makes this impossible (since you can't have constructors in a union).

The code is also simpler without this useless class.

Comment 1

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

Attachment: patch

The patch is largely code movement. We need to move some inlines out of jsobj.h since they now depend on fields of jsscope.h, which can't be included. I think it's a good general goal to move as many inlines out of .h files into inlines files, and this patch moves in that direction.

Jason, I realized that the test case we found for the bound function bug doesn't actually work. I fixed the bug, but I'm unable to find a real test case.

Comment 2

[Luke Wagner [:luke]]

Yipee, this sucker always confused me.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> I think it's a good general goal to move as many inlines out of .h files into
> inlines files

Really?  My understanding was that inlines files were a necessary evil at best.  Moving stuff that doesn't have to be in them into them sounds to me like a recipe for lots of typing without any extra clarity.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 524741 [details] [diff] [review]
patch

Thanks. r=me.

njn: I don't know if there's a sane way to write C++ with so very many inlines. Having two tiers, with the inline definitions in the second tier, solves some of the problems. Not all of course.

If possible, it's better to untangle the rat's next and make all the dependencies one-way. If!

Comment 5

[Luke Wagner [:luke]]

(In reply to comment #3)
> Really?  My understanding was that inlines files were a necessary evil at best.
>  Moving stuff that doesn't have to be in them into them sounds to me like a
> recipe for lots of typing without any extra clarity.

Necessary evil: yes.  But the evil isn't just resolving dependencies, its slow compile times.  In theory, we are including *inlines.h files a lot less than their *.h counterparts thus reducing the size of most translation units.  Really, I think we need a big-time disentangling of all our *.h files.

Comment 6

[Nicholas Nethercote [inactive]]

I suspect if all the inlined functions were moved into the inlines.h files then those files would end up being #included a lot more -- after all, you have to #include something -- and compile times wouldn't change much.  I'd be happy to be proven wrong, though.

Comment 7

[Brendan Eich [:brendan]]

js*inlines.h files are never included by .h files other than ones whose names match js*inlines.h (same should go for the Googly StudlyCaps-inl.h convention).

This is the point: inline/-inl.h files are implementations, not interfaces.

Not sure this helps with compile time but I could see us increasing compile time by putting too many inline method bodies in non-inline/-inl .h files.

Good to see JSObjectMap go, it is an obsolete bit of ancient history.

/be

Comment 8

[Brendan Eich [:brendan]]

I think I mentioned irl to Bill my pref to split .h files up into smaller files, which can also hurt compile time, but should help us avoid over-including (so may net out as a win).

Someone please file that bug. With fast release cycles, I think we should make continuous improvements according to an agreed-upon plan, without any big "flag days" where we rename the universe. That seems likelier to get results than the flag-day thing (of course, we need the written-down plan first).

StudlyCaps.{h,cpp} and StudlyCaps-inl.h seems a fine convention to rename toward, since we are using it already in methodjit at least.

/be

Comment 9

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

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

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

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

Comment 11

[Eric Shepherd [:sheppy]]

This article is updated:

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JSObject

Looking at the code, it appears that at some point (apparently in the hazy past), JSObjectOps went away. Is that true? If so, I can go ahead and nuke:

https://developer.mozilla.org/en/JSObjectOps

Plus its subpages and adjust pages that link to it.