Last Comment Bug 648647 - Eliminate JSObjectMap
: Eliminate JSObjectMap
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-08 14:26 PDT by Bill McCloskey (:billm)
Modified: 2011-05-02 12:37 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (39.02 KB, patch)
2011-04-08 14:30 PDT, Bill McCloskey (:billm)
jorendorff: review+
Details | Diff | Review

Description Bill McCloskey (:billm) 2011-04-08 14:26:49 PDT
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 (:billm) 2011-04-08 14:30:41 PDT
Created attachment 524741 [details] [diff] [review]
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] 2011-04-08 16:54:23 PDT
Yipee, this sucker always confused me.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-04-10 23:21:15 PDT
(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] 2011-04-11 09:56:19 PDT
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] 2011-04-11 10:31:49 PDT
(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 [:njn] (on vacation until July 11) 2011-04-11 13:53:12 PDT
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] 2011-04-11 14:05:00 PDT
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] 2011-04-11 14:08:06 PDT
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 (:billm) 2011-04-20 10:56:27 PDT
http://hg.mozilla.org/tracemonkey/rev/fbf611d8bec3
Comment 10 Chris Leary [:cdleary] (not checking bugmail) 2011-04-26 15:20:55 PDT
http://hg.mozilla.org/mozilla-central/rev/fbf611d8bec3
Comment 11 Eric Shepherd [:sheppy] 2011-04-26 19:23:00 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.