1063233 - JS::ubi::Node specializations for SpiderMonkey types should live with those types' definitions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

JS::ubi::Node is designed to allow the code that implements the ubi::Node methods for a particular referent type live with the implementation of that referent type. This lets us keep all the code that understands a particular type in one place.

At the moment, all JS::ubi::Node support for SpiderMonkey types is in js/public/UbiNode.h and js/src/vm/UbiNode.cpp. This means that js/public/UbiNode.h includes incomplete declarations for internal SpiderMonkey types like js::Shape and js::BaseShape. This is dumb, and dumb in a way that ubi::Node was designed to avoid.

Comment 1

[Jim Blandy :jimb]

Attachment: Move JS::ubi::Node support for js::LazyScript to sit next to the rest of LazyScript's implementation.

Comment 2

[Jim Blandy :jimb]

Attachment: Move JS::ubi::Node support for js::Shape and js::BaseShape next to their declarations and definitions.

Comment 3

[Jim Blandy :jimb]

Attachment: Move JS::ubi::Node support for js::jit::JitCode next to where it is declared/defined.

Comment 4

[Jim Blandy :jimb]

Attachment: Move JS::ubi::Node support for js::types::TypeObject next to where it is declared/defined.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8484625 [details] [diff] [review]
Move JS::ubi::Node support for js::Shape and js::BaseShape next to their declarations and definitions.

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

::: js/src/vm/Shape.cpp
@@ +1578,5 @@
> +template<> const jschar TracerConcrete<js::BaseShape>::concreteTypeName[] =
> +    MOZ_UTF16("js::BaseShape");
> +
> +} // namespace ubi
> +} // namespace JS

Why does this decl need to be in the namespace but the other does not? Please use the same style as JS::ubi::TracerConcreate<js::Shape> here if at all possible.

Comment 6

[Jim Blandy :jimb]

(In reply to Terrence Cole [:terrence] from comment #5)
> Why does this decl need to be in the namespace but the other does not?
> Please use the same style as JS::ubi::TracerConcreate<js::Shape> here if at
> all possible.

I botched it. Thanks for catching that.

Comment 7

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04d42b23a489
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1913c07322
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e3d887faa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6db27e6240

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this and bug 1063247 out for non-unified Windows build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04be894027e6
https://tbpl.mozilla.org/php/getParsedLog.php?id=47507112&tree=Mozilla-Inbound

Comment 9

[Jim Blandy :jimb]

Attachment: Provide default definitions for optional JS::ubi::Node methods in Base.

Comment 10

[Jim Blandy :jimb]

Attachment: Move JS::ubi::Node specializations for SpiderMonkey types closer to those types' definitions.

Okay, I've got a clean (well, starred) try push for this one that is fully non-unified:
https://tbpl.mozilla.org/?tree=Try&rev=cb4409b1980a

Comment 11

[Jim Blandy :jimb]

Attachment: Work around bug 1066193 (compiler bug) by providing an explicit 'this->'.

Not requesting review yet; let's see what happens in bug 1066193.

Comment 12

[Jim Blandy :jimb]

Try push with the bug 1066193 workaround:
https://tbpl.mozilla.org/?tree=Try&rev=f251d8b8497a

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 8488278 [details] [diff] [review]
Provide default definitions for optional JS::ubi::Node methods in Base.

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

Okay. It's a bit worrisome that there are contexts where the actual defs aren't available, but I guess that's the cost of diaspora with C++'s build model.

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 8488280 [details] [diff] [review]
Move JS::ubi::Node specializations for SpiderMonkey types closer to those types' definitions.

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

Nice!

Comment 15

[Jim Blandy :jimb]

Thanks for the reviews!

Because we've had such problems in this bug, here's another Try push, this time without the unification-disabling patch. Gotta try everything both ways.

https://tbpl.mozilla.org/?tree=Try&rev=d70d3d0b03d2

Comment 16

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc35048535c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbc9c3777e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf043246967d

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc35048535c8
https://hg.mozilla.org/mozilla-central/rev/4bbc9c3777e5
https://hg.mozilla.org/mozilla-central/rev/cf043246967d