1429206 - Move a few more js/src/*.{h,cpp} files into subdirectories

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jason Orendorff [:jorendorff]]

I wrote patches for a few of these over the vacation. They don't bitrot patches as much as I expected, so we can just land them.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Use js/TypeDecls.h instead of redeclaring certain types

This adds JS::Zone to TypeDecls.h. Arguably that is pretty borderline, but
even though the Zone type is only used in a dozen or so places in Gecko,
it's a central concept.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Rename JSObject -> JS::Object (and several others)

The old type names still exist (as aliases for the new ones), and we still
mainly use the old names rather than the new ones.

The affected types are JSContext, JSCompartment, JSFunction, JSObject, JSScript,
and JSString.

We have some places where functions with the same name are declared
in multiple namespaces. In a few of those cases, moving JSObject into the
JS namespace interacts with C++ argument-dependent lookup to cause issues,
which I fixed by adding `::` before some function names.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: Rename jsobj* -> obj/Object*

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 4: Rename jsfun* -> vm/Function*

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 5: Rename jsscript* -> vm/Script*

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 6: Rename jscompartment* -> vm/Compartment*

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 7: Rename jscntxt* -> vm/Context*

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 8: Rename jsatom* -> vm/Atom*

Comment 9

[Jason Orendorff [:jorendorff]]

That's it for this batch. In a follow-up, I also want to move:

*   vm/DateObject* -> builtin/

*   vm/Shape* -> obj/, along with ArrayObject, ArrayBufferObject,
    NativeObject, TypedArrayObject, and probably some others
    The idea is that the core classes and methods about how we
    represent objects in memory go in js/src/obj.

The remaining headers in js/src are a mix, but it'd be more work to sort them:

*   jsalloc.h, jsapi.h, jsfriendapi.h, jstypes.h, and 7 others are public headers.
    They should all be moved to js/public, with or without renaming them.

*   jsarray* should be taken apart, the contents split across several headers.
    It's more work than just renaming the files.

*   jsgc* -> gc/

*   json.* -> builtin/

*   jsbool*.h, jsdate.h, jsexn.h, jsiter.h, jsnum.h, jsstr.h
    All of these could plausibly go in builtin/ or vm/.

*   jshashutil.h -> ds/ probably

*   jsopcode*.h belongs in vm/ ...but there's already a vm/Opcodes.h

*   jsdtoa.h, jsnspr.h, jsutil.h, jswin.h, NamespaceImports.h are low-level
    and/or defy categorization; they could be in their own directory maybe.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

To help understanding, could you provide the reasoning for having builtin/, vm/ and new directories you're introducing here, as well as criteria that would help us deciding where to put files, please? I've always found it confusing to decide (builtin/ vs vm/) and I would love it if there was a detailed explanation somewhere of what must go where. Thanks!

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8941921 [details] [diff] [review]
Part 1: Use js/TypeDecls.h instead of redeclaring certain types

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

Yes, much nicer.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8941923 [details] [diff] [review]
Part 2: Rename JSObject -> JS::Object (and several others)

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

Do you think it makes sense to do a mass search-and-replace at some point?

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8941924 [details] [diff] [review]
Part 3: Rename jsobj* -> obj/Object*

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

::: js/src/gc/Marking.h
@@ +178,5 @@
>  inline void
>  MakeAccessibleAfterMovingGC(void* anyp) {}
>  
>  inline void
> +MakeAccessibleAfterMovingGC(JSObject* obj); // Defined in objects/JSObject-inl.h.

Defined in obj/Object-inl.h.

::: js/src/json.cpp
@@ +13,5 @@
>  #include "jsarray.h"
>  #include "jsatom.h"
>  #include "jscntxt.h"
>  #include "jsnum.h"
> +

Remove the blank line here

Comment 14

[Jan de Mooij [:jandem]]

Hm I don't understand part 4 - doesn't JSFunction (JS::Function) belong in obj/ instead of vm/? Why would TypedArrayObject and ArrayObject be in obj/ but not Function?

It would be nice to have a simple rule here; maybe everything object related + everything with a Class* should be in obj/?

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8941926 [details] [diff] [review]
Part 5: Rename jsscript* -> vm/Script*

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

::: js/src/vm/Script-inl.h
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef vm_JSScript_inl_h

vm_Script_inl_h and below

::: js/src/vm/Script.h
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* JS script descriptor. */
>  
> +#ifndef vm_JSScript_h

And here.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8941929 [details] [diff] [review]
Part 8: Rename jsatom* -> vm/Atom*

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

Nice.

::: js/src/vm/Atom-inl.h
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef jsatominlines_h

Nit: vm_Atom_inl_h and vm_Atom_h in vm/Atom.h

Comment 17

[Jason Orendorff [:jorendorff]]

> Hm I don't understand part 4 - doesn't JSFunction (JS::Function) belong in obj/ instead of vm/? Why would TypedArrayObject and ArrayObject be in obj/ but not Function?
>
> It would be nice to have a simple rule here; maybe everything object related + everything with a Class* should be in obj/?

OK, discussed this with jandem and evilpies. Forget obj/, I'll move jsobj.h -> vm/Object.h.

I wrote up our other conclusions in the wiki: https://wiki.mozilla.org/JavaScript:SpiderMonkey:Directories

Comment 18

[Jason Orendorff [:jorendorff]]

> Do you think it makes sense to do a mass search-and-replace at some point?

Yes.

Comment 19

[Jason Orendorff [:jorendorff]]

Having said that...

Interestingly, renaming JSObject to JS::Object in patch 2 causes problems. SM compiles but fails this jsapi-test:

testIncrementalWeakCacheSweeping
Assertion failure: !js::gc::EdgeNeedsSweepUnbarrieredSlow(&obj), at /Users/jorendorff/work/gecko/js/src/build_DBG.OBJ/dist/include/js/HeapAPI.h:595

I imagine this is due to C++ argument-dependent lookup, but I don't know an easy way to track down something like that. I am going to push everything else to try server, and set patch 2 aside for now.

Comment 20

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4c02c33d4df0cf6f7a76f2ba6e9c7a351ee5b75

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 8941925 [details] [diff] [review]
Part 4: Rename jsfun* -> vm/Function*

Cancelling review since I assume there will be an updated patch.

Comment 22

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=151bbfee8d015d658588608c5adad69ac9ac57d1

Comment 23

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49b5f588b14e09973239f4c550e32cdfc9223f1d

Comment 24

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec904300b775a82158b949e8d5c0c407cc4418b3

Comment 25

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e0f41932c9aca655be2121ad4464ad2ba551d4

Comment 26

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Use js/TypeDecls.h instead of redeclaring certain types

This adds JS::Zone to TypeDecls.h. Arguably that is pretty borderline, but
even though the Zone type is only used in a dozen or so places in Gecko,
it's a central concept.

Comment 27

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: Rename jsobj* -> vm/JSObject*

Comment 28

[Jason Orendorff [:jorendorff]]

Comment on attachment 8941923 [details] [diff] [review]
Part 2: Rename JSObject -> JS::Object (and several others)

Obsoleting this patch: This change breaks everything, and tracking down why is too hard right now. Maybe someday.

Comment 29

[Jason Orendorff [:jorendorff]]

Attachment: Part 4: Rename jsfun* -> vm/JSFunction*

Comment 30

[Jason Orendorff [:jorendorff]]

Attachment: Part 5: Rename jsscript* -> vm/JSScript*

Comment 31

[Jason Orendorff [:jorendorff]]

Attachment: Part 6: Rename jscompartment* -> vm/JSCompartment*

Comment 32

[Jason Orendorff [:jorendorff]]

Attachment: Part 7: Rename jscntxt* -> vm/JSContext*

Comment 33

[Jason Orendorff [:jorendorff]]

Attachment: Part 8: Rename jsatom* -> vm/JSAtom*

Comment 34

[Jason Orendorff [:jorendorff]]

OK. Part 2 is gone. Part 1 stays, even though it is really preparation for part 2, because it's a good change anyway.

Comment 35

[Jason Orendorff [:jorendorff]]

It will feel weird typing `#include "vm/JSObject.h"` at first, but that will wear off quickly.

Comment 36

[Jan de Mooij [:jandem]]

Comment on attachment 8950634 [details] [diff] [review]
Part 8: Rename jsatom* -> vm/JSAtom*

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

Nice!

Comment 37

[Jan de Mooij [:jandem]]

Note that JSString is currently in vm/String.h, so we could have vm/Function.h instead of vm/JSFunction.h I don't care too much though; either one is better than jsfun.h :)

Comment 38

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/301c61b644c24acc65c4d0b379736d1d3c7999a6
Bug 1429206 - Part 1: Use js/TypeDecls.h instead of redeclaring certain types. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eabb74b1c3bd181ff74c9d41003bc2b9aee6fe68
Bug 1429206 - Part 3: Rename jsobj* -> vm/JSObject*. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/23d76102a2e8acd0e79c26faa44eec833db58529
Bug 1429206 - Part 4: Rename jsfun* -> vm/JSFunction*.

https://hg.mozilla.org/integration/mozilla-inbound/rev/711c111e333087942d9e5c7310a342ddffbcfabb
Bug 1429206 - Part 5: Rename jsscript* -> vm/JSScript*. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/212a88df0f3d598edf04df824d568667c20c5fec
Bug 1429206 - Part 6: Rename jscompartment* -> vm/JSCompartment*. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/609eb4880073d3b1ff1a198ec68737dce50bcb3f
Bug 1429206 - Part 7: Rename jscntxt* -> vm/JSContext*. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0054d892b106cfc60874dc1662a7278c74108b03
Bug 1429206 - Part 8: Rename jsatom* -> vm/JSAtom*. r=jandem.

Comment 39

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/301c61b644c2
https://hg.mozilla.org/mozilla-central/rev/eabb74b1c3bd
https://hg.mozilla.org/mozilla-central/rev/23d76102a2e8
https://hg.mozilla.org/mozilla-central/rev/711c111e3330
https://hg.mozilla.org/mozilla-central/rev/212a88df0f3d
https://hg.mozilla.org/mozilla-central/rev/609eb4880073
https://hg.mozilla.org/mozilla-central/rev/0054d892b106