642381 - Move LazilyConstructed (and dependencies) out of jstl and into mfbt

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Move LazilyConstructed out of jstl and into mfbt

LazilyConstructed isn't at all specific to SpiderMonkey, and it's already used by random Gecko code, so there's existing motivation for it to live in an officially shared location.

It would usually be silly to pull so many busy people into such a superficially trivial bug, but this is the first interesting code added to mfbt, and mfbt's first use in js/src.  CC'ing people who I think might have an opinion on the way things are shaping up.  Questions for all the CC'd

 (1) Does anyone object to sharing code like it's done in the attached patch?

 (2) SM folk: does anyone object to use of namespace "mozilla" in js/src?  If so, what's the counter-proposal?

The last issues are for the more build/distribution-inclined among the CC'd.  The issues are important because mfbt/ will need to be distributed along with a standalone SpiderMonkey.

 (3) LazilyConstructed depends on exact-sized integer types.  I still hold out hope for bug 465641 and bug 579517, especially now that we're moving to VC10, but we may never be able to use stdint types in public headers.  mfbt might leak into public headers, so the forthcoming patch plays it safe and imports the stdin[sic] types (stdint types minus the "_t", e.g. "int32") from jstypes.h.  It's wrong that mfbt is logically "lower level" than js/src yet relies on a js/src header, but I don't any gain in refactoring this code given (5) below.  Any objections?

 (4) LazilyConstruct depends on a strong ASSERT() macro.  There's already bug 576052 on using a not-silly strong assert in Gecko, so the forthcoming patch kills that bird too by adding MOZ_ASSERT() to mfbt.  MOZ_ASSERT() is identical to JS_ASSERT().  Objections?

 (5) LazilyConstruct depends on an "extern" function, meaning it relies on a definition in a .cpp file.  (The dependency is through the MOZ_ASSERT() macro, which uses a assertion-failed function that can't be inlined without pulling in several large libc headers.)  This is the nastiest of the issues: we will need to compile .cpp files in mfbt/ and somehow link them with libmozjs and libxul.  I think the easiest way to do this is to compile a libmfbt.a static lib which is linked into libmozjs(.a|.so).  That is, in practice mfbt will always be part of js/src.  This is why I don't care so much about the backwards dependency of mfbt on jstypes.h in (3); to reverse the edge would require a lot of refactoring for no practical gain, AFAICT.  Objections?

(All that said, I punted on (5) by having MOZ_ASSERT() use the existing JS_Assert() function.  Ha!  This will come up again soon though, at least with bug 507718, so I would like to have a tentative plan in place.)

Comment 1

[David Mandelin [:dmandelin]]

(In reply to comment #0)
> Created attachment 519844 [details] [diff] [review]
> Move LazilyConstructed out of jstl and into mfbt
> 
> LazilyConstructed isn't at all specific to SpiderMonkey, and it's already used
> by random Gecko code, so there's existing motivation for it to live in an
> officially shared location.
> 
> It would usually be silly to pull so many busy people into such a superficially
> trivial bug, but this is the first interesting code added to mfbt, and mfbt's
> first use in js/src.  CC'ing people who I think might have an opinion on the
> way things are shaping up.  Questions for all the CC'd
> 
>  (1) Does anyone object to sharing code like it's done in the attached patch?

I think we should absolutely share modern C++ utility code between SM and the browser. 

I'm not sure about the best way to do it, though. It looks like WebKit puts that kind of thing in JavaScriptCore/wtf. I think the analogous place for us in the current tree is js/src/mfbt. That's certainly the most convenient place from a SM hacker's point of view, so that's my initial preference. :-) I think that solves a bunch of other problems; I don't know if it creates any on the Gecko side.

>  (2) SM folk: does anyone object to use of namespace "mozilla" in js/src?  If
> so, what's the counter-proposal?

It seems OK. But maybe 'mfbt' instead? Especially if we are using js/src/mfbt.

> The last issues are for the more build/distribution-inclined among the CC'd. 
> The issues are important because mfbt/ will need to be distributed along with a
> standalone SpiderMonkey.
> 
>  (3) LazilyConstructed depends on exact-sized integer types.  I still hold out
> hope for bug 465641 and bug 579517, especially now that we're moving to VC10,
> but we may never be able to use stdint types in public headers.  mfbt might
> leak into public headers, so the forthcoming patch plays it safe and imports
> the stdin[sic] types (stdint types minus the "_t", e.g. "int32") from
> jstypes.h.  It's wrong that mfbt is logically "lower level" than js/src yet
> relies on a js/src header, but I don't any gain in refactoring this code given
> (5) below.  Any objections?

What exactly blocks us from using stdint types now? If it's just MSVC, it seems we should be able to create a copy of enough of stdint to get things to work there. I'd like to get on the standard stuff it all possible.

Otherwise, if we used js/src/mfbt, then the JS types header definitions could live there, and other mfbt could depend on them without any weirdness.

>  (4) LazilyConstruct depends on a strong ASSERT() macro.  There's already bug
> 576052 on using a not-silly strong assert in Gecko, so the forthcoming patch
> kills that bird too by adding MOZ_ASSERT() to mfbt.  MOZ_ASSERT() is identical
> to JS_ASSERT().  Objections?

I approve of not-silly asserts.

>  (5) LazilyConstruct depends on an "extern" function, meaning it relies on a
> definition in a .cpp file.  (The dependency is through the MOZ_ASSERT() macro,
> which uses a assertion-failed function that can't be inlined without pulling in
> several large libc headers.)  This is the nastiest of the issues: we will need
> to compile .cpp files in mfbt/ and somehow link them with libmozjs and libxul. 
> I think the easiest way to do this is to compile a libmfbt.a static lib which
> is linked into libmozjs(.a|.so).  That is, in practice mfbt will always be part
> of js/src.  This is why I don't care so much about the backwards dependency of
> mfbt on jstypes.h in (3); to reverse the edge would require a lot of
> refactoring for no practical gain, AFAICT.  Objections?
> 
> (All that said, I punted on (5) by having MOZ_ASSERT() use the existing
> JS_Assert() function.  Ha!  This will come up again soon though, at least with
> bug 507718, so I would like to have a tentative plan in place.)

This problem goes away if use js/src/mfbt, right?

Comment 2

[Luke Wagner [:luke]]

(In reply to comment #1)
> I think we should absolutely share modern C++ utility code between SM and the
> browser. 
> 
> I'm not sure about the best way to do it, though. It looks like WebKit puts
> that kind of thing in JavaScriptCore/wtf. I think the analogous place for us in
> the current tree is js/src/mfbt. That's certainly the most convenient place
> from a SM hacker's point of view, so that's my initial preference. :-) I think
> that solves a bunch of other problems; I don't know if it creates any on the
> Gecko side.

This was already discussed to a degree on an email thread.  The tentative proposal was to put most stuff in a mozilla/mfbt including some libraries aimed to be C++0x-like.  Then there would be a mozilla/js/mfbt to hold things (viz., js::Hash{Map,Table}, js::Vector) which, while shareable by all Mozilla, are key to the JS engine and thus owned by the JS module.  This goal of this separation is to avoid conflicts, e.g., between having code be C++0x-like and having code be optimally suited for the JS engine (which, as we've seen, are often at odds).  js/mfbt of course would include mfbt and so, from a JS pov, both would be accessible.

As for js/mfbt vs. js/src/mfbt, its syntactic, but I would prefer js/mfbt since we'd like to eventually like to prevent all external dependencies into the JS privates (which is, roughly, js/src).

Comment 3

[Luke Wagner [:luke]]

(In reply to comment #2)
> The tentative proposal

I should clarify that by "tentative" I mean "I proposed and noone has raised any objections yet"... very tentative ;-)

Comment 4

[Luke Wagner [:luke]]

(In reply to comment #0)
>  (2) SM folk: does anyone object to use of namespace "mozilla" in js/src?  If
> so, what's the counter-proposal?

Seems fine to me.  For minimal syntax in js, we can just have:

namespace js { using namespace mozilla; }

in a central js header.  This lets you write js::X instead of mozilla::X and (unqualified) X if you have done 'using namespace js' which effectively makes mozilla mfbt part of js (which, really, is what we want to do here).

>  (3) LazilyConstructed depends on exact-sized integer types.  I still hold out
> hope for bug 465641 and bug 579517, especially now that we're moving to VC10,
> but we may never be able to use stdint types in public headers.  mfbt might
> leak into public headers, so the forthcoming patch plays it safe and imports
> the stdin[sic] types (stdint types minus the "_t", e.g. "int32") from
> jstypes.h.  It's wrong that mfbt is logically "lower level" than js/src yet
> relies on a js/src header, but I don't any gain in refactoring this code given
> (5) below.  Any objections?

Could we just hoist jstypes.h and dependences into mozilla/mfbt?  It would have to be C-includable (so, types would still be in the global namespace).  I think I recall Brendan saying that we can kill all the JStypeN in lieu of new shorter typeN typedefs.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #2)
> (In reply to comment #1)
> > I think we should absolutely share modern C++ utility code between SM and the
> > browser. 
> > 
> > I'm not sure about the best way to do it, though. It looks like WebKit puts
> > that kind of thing in JavaScriptCore/wtf. I think the analogous place for us in
> > the current tree is js/src/mfbt. That's certainly the most convenient place
> > from a SM hacker's point of view, so that's my initial preference. :-) I think
> > that solves a bunch of other problems; I don't know if it creates any on the
> > Gecko side.
> 
> This was already discussed to a degree on an email thread.  The tentative
> proposal was to put most stuff in a mozilla/mfbt including some libraries aimed
> to be C++0x-like.  Then there would be a mozilla/js/mfbt to hold things (viz.,
> js::Hash{Map,Table}, js::Vector) which, while shareable by all Mozilla, are key
> to the JS engine and thus owned by the JS module.  This goal of this separation
> is to avoid conflicts, e.g., between having code be C++0x-like and having code
> be optimally suited for the JS engine (which, as we've seen, are often at
> odds).  js/mfbt of course would include mfbt and so, from a JS pov, both would
> be accessible.

I don't care too much where these things live. But I do want a simple, easy, build process. I really envy "Tools/Scripts/build-jsc" and "scons sample=shell". If the makefiles can just figure everything out about mfbt for SM then it seems fine.

> As for js/mfbt vs. js/src/mfbt, its syntactic, but I would prefer js/mfbt since
> we'd like to eventually like to prevent all external dependencies into the JS
> privates (which is, roughly, js/src).

We don't really have a public/private separation yet, do we? But agreed, it would be great to move in that direction.

Comment 6

[Luke Wagner [:luke]]

(In reply to comment #5)
> We don't really have a public/private separation yet, do we? But agreed, it
> would be great to move in that direction.

Officially, jsapi.h/jsdbgapi.h are public, the rest is private.  I have seen several patches that have been incrementally chipping away at private-#includers.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

FWIW, and this is a bit bikesheddy, I strongly prefer namespace "mozilla" over "mfbt". Acronyms and codenames in our source code make it harder for people to understand.

Comment 8

[David Mandelin [:dmandelin]]

(In reply to comment #7)
> FWIW, and this is a bit bikesheddy, I strongly prefer namespace "mozilla" over
> "mfbt". Acronyms and codenames in our source code make it harder for people to
> understand.

I think it's a good point and not bikesheddy. I agree that making things harder to understand for no good reason is bad. (I don't even know what "mfbt" stands for, myself. (And yes, I did advocate it a few comments ago.)) And I guess most files will just say "using namespace mozilla;" so that the 3 extra chars really cost no typing.

Comment 9

[Andreas Gal :gal]

+1, please use mozilla, not mfbt.

Comment 10

[Luke Wagner [:luke]]

comment 0 proposed 'mozilla' as the namespace, 'mfbt' as the source directory, so I don't think anyone disagrees.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Seems fine to me.  For minimal syntax in js, we can just have:
> 
> namespace js { using namespace mozilla; }
> 

Seems OK, but we'd have to be careful about naming divergent,
non-std:: types in mfbt/ and js/mfbt/ if/when both need to fork.

> Could we just hoist jstypes.h and dependences into mozilla/mfbt?  It would have
> to be C-includable (so, types would still be in the global namespace).  I think
> I recall Brendan saying that we can kill all the JStypeN in lieu of new shorter
> typeN typedefs.

Yes, that's logically what attachment 519844 [details] [diff] [review] does: creates
mfbt/Types.h that provides all the necessary defitions for SM and
Gecko.  It just so happens that the current implementation is
|#include "jstypes.h"| ;).

I'd like to actually, not just logically, move all stuff into mfbt/
too, but I don't see what we would gain by doing that right now.  I
think a good carrot for that work would be something like bug 465638,
with the file shuffling possibly being a followup patch.  The one
sticky wicket with that work is that mfbt/ will depend on the result
of js/src/configure.in, which is a bit weird.  Doesn't bug me too much
though.

(In reply to comment #5)
> I don't care too much where these things live. But I do want a simple, easy,
> build process. I really envy "Tools/Scripts/build-jsc" and "scons
> sample=shell". If the makefiles can just figure everything out about mfbt for
> SM then it seems fine.

Agreed, and I haven't seen any indication yet that we can't make this
all work magically.  (Distributing mfbt/ and js/mfbt/ along with
js/src/ is a bit trickier, but in all likelihood Someone Else's
Problem.)

(In reply to comment #6)
> Officially, jsapi.h/jsdbgapi.h are public, the rest is private.  I have seen
> several patches that have been incrementally chipping away at
> private-#includers.

One of them is attachment 519844 [details] [diff] [review]!  If it wasn't clear, that patch removes
Gecko's dependency on jstl.h, AFAICT.

(In reply to comment #10)
> comment 0 proposed 'mozilla' as the namespace, 'mfbt' as the source directory,
> so I don't think anyone disagrees.

Aye, namespace "mozilla" was indeed proposed based on Rob's earlier suggestion.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Move LazilyConstructed out of jstl and into mfbt, rebased

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Hoist LazilyConstructed into mfbt, eliminate Gecko's use of jstl

This patch is pretty trivial except for mfbt/Types.h.  Pulling in Jason and Brendan to evaluate potential effects re: embedding (should be none).

As noted in comment 11, we perhaps eventually might want to eliminate jstypes.h in favor of mfbt/Types.h, but I don't see a lot of gain for that work atm.

Comment 14

[Andreas Gal :gal]

I was really hoping we would not go with mfbt.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We did what comment 10 describes.  No one objected.

Comment 16

[Andreas Gal :gal]

Oh. Ok. I guess thats a reasonable compromise. mozilla as directory name would be confusing, I agree. Ok, forget everything I said after good morning and carry on.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 523513 [details] [diff] [review]
Hoist LazilyConstructed into mfbt, eliminate Gecko's use of jstl

Awesome work forging the way!

Ditto bug 647011 comment 6 concerning the handling of namespaces.

Since you are touching a lot of the uses anyway, this seems like a good time to address the somewhat haphazard naming of LazilyConstructed/Conditionally (my fault, of course :)

For one thing: there doesn't seem to be any reason not to roll Conditionally into LazilyConstructed.

For another: I've always thought that if I was going to name them again I'd just go with Maybe<T>.  That matches well the growing use of 'maybe' in SM names to indicate optionality.  And of course Haskell has Maybe.  And its short.  I'm sorry to create any more work for you; if you'd prefer (assuming other people agree on the name) I can take your patch and do the grunt renaming.

>diff --git a/js/src/jstl.h b/js/src/jstl.h
> #include "jsbit.h"
> #include "jsstaticcheck.h"
>+#include "mozilla/Util.h"
> 
> #include <new>
> #include <string.h>

Since jstl.h was really only originally intended for meta-functions (namespace js::tl), and perhaps we could not #include mozilla/Util.h here, but in jsutil.h (which was part of the above suggestion).

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #17)
> Ditto bug 647011 comment 6 concerning the handling of namespaces.

Yep.

> For one thing: there doesn't seem to be any reason not to roll Conditionally
> into LazilyConstructed.

Sure.  MXR says that Conditionally has 3 users, so no worries.

> For another: I've always thought that if I was going to name them again I'd
> just go with Maybe<T>.

Hmmmm.  I had been thinking of LazilyConstructed as more of an optimization than an option type, but I guess it's really both.

Maybe<T> suits me; I tried to use the "Maybe" qualifier in Gecko a while back for a similar pattern and was shot down, but if JS folks are happy with Maybe<T>, I certainly am ;).

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Remove js::Conditionally

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Rename LazilyConstructed to Maybe

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Hoist Maybe into mfbt and eliminate Gecko's use of jstl

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 524473 [details] [diff] [review]
part 1: Remove js::Conditionally

>+    LazilyConstructed<AutoUnlockAtomsCompartment> maybeUnlockAtomsCompartment;
>+    if (cx->compartment == rt->atomsCompartment &&
>+        rt->atomsCompartmentIsLocked)
>+        maybeUnlockAtomsCompartment.construct(cx);

Multi-line conditionals need { } around branch.  In this case, though, you should be able to fit the whole condition on a single line (SM style now allows up to 99).

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 524475 [details] [diff] [review]
part 2: Rename LazilyConstructed to Maybe

Much appreciated!

One nit: could you reflow the Maybe template's comment to 80 columns?

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 524477 [details] [diff] [review]
part 3: Hoist Maybe into mfbt and eliminate Gecko's use of jstl

Righteous

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #22) 
> Multi-line conditionals need { } around branch.  In this case, though, you
> should be able to fit the whole condition on a single line (SM style now allows
> up to 99).

Moved to single line.

(In reply to comment #23) 
> One nit: could you reflow the Maybe template's comment to 80 columns?

Done.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

sr ping.  (Maybe it's not needed here?)

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

sr ping.

Comment 28

[Andreas Gal :gal]

I don't think sr is needed here. Blame me if anyone complains.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 524477 [details] [diff] [review]
part 3: Hoist Maybe into mfbt and eliminate Gecko's use of jstl

Will do.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/tracemonkey/rev/2effdc4e447d
http://hg.mozilla.org/tracemonkey/rev/c8dcc08a4a8a
http://hg.mozilla.org/tracemonkey/rev/3dd6ec45084c

Comment 31

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

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2effdc4e447d
http://hg.mozilla.org/mozilla-central/rev/c8dcc08a4a8a
http://hg.mozilla.org/mozilla-central/rev/3dd6ec45084c

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 524477 [details] [diff] [review]
part 3: Hoist Maybe into mfbt and eliminate Gecko's use of jstl

>diff --git a/mfbt/Types.h b/mfbt/Types.h
>new file mode 100644
>--- /dev/null
>+++ b/mfbt/Types.h
>+ * Portions created by the Initial Developer are Copyrigght (C) 2011

Rigght. There is a template without typos, please use it.

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> Rigght. There is a template without typos, please use it.
https://www.mozilla.org/MPL/boilerplate-1.1/ FWIW

The formatting is wrong too...

Comment 34

[Eric Shepherd [:sheppy]]

I've been looking at this for a while, and I'm not able to figure out what needs a documentation update here. This appears to be changes to some C++ templates which are not documented.

Should they be?

Comment 35

[Shawn Wilsher :sdwilsh]

(In reply to comment #34)
> I've been looking at this for a while, and I'm not able to figure out what
> needs a documentation update here. This appears to be changes to some C++
> templates which are not documented.
> 
> Should they be?
These are platform APIs that people [can] use throughout the code base.  This documentation would target a similar audience as https://developer.mozilla.org/en/XPCOM_string_guide

Comment 36

[Eric Shepherd [:sheppy]]

fwiw, this is one of those things I'd rather pretty much anyone but me write about, because C++ templates and I are not on speaking terms. :)

Comment 37

[Jason Orendorff [:jorendorff]]

It's not fair to call this dev-doc-needed (which puts it on sheppy's radar) unless you're willing to do the work.

I am happy looking at the source files, since the comments are unusually thorough and clear. Plus the implementation is also right there in case I have any questions. If anyone wants to document it on mdn, feel free (as always).

Comment 38

[Jeff Walden [:Waldo]]

I'd suggest MDN should act as a funnel ("what do you need?  this file and data structure are what you want"), and the source code should have comments and examples which act as explanation.  That would avoid much repetitiveness, and it sounds like (per last part of comment 37) we're already there on the source code side pretty much.  In this case sheppy or whoever would have to do almost nothing with C++ templates to document this.

Comment 39

[Jeff Walden [:Waldo]]

Following up on what I proposed in comment 38, I've written this document to point people to the header files, with substantial comments by the implementations to describe APIs, uses, and functionality:

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

The class here currently lives in Util.h, which makes it hard for someone to guess where that might live.  Bug 713082 should split this out into a (say) DebugOnly.h header to make it easier to guess where this functionality would live.