Closed Bug 1460341 Opened Last year Closed Last year

Remove GCPolicy<T>::initial

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(2 files, 3 obsolete files)

The GCPolicy<T>::initial() method returns nullptr for pointers or T() for non-pointers in almost all cases, with the exeception of jsids where it returns JSID_VOID.  If we can make jsid() be JSID_VOID then we can use T() and get rid of this method entirely.
See Also: → 1459577
Patch to make value initialisation of jsid yield JSID_VOID.  Value initialisation zeros the memory so this means making JSID_VOID be represented by a string-tagged jsid with a null pointer.  This wasn't an allowed value before so I *think* this is OK.
Attachment #8974443 - Flags: review?(jdemooij)
Remove GCPolicy<T>::initial and replace with T().
Attachment #8974444 - Flags: review?(sphink)
Could you add static_asserts verifying that T is either a pointer, or a class with non-trivial constructor?  Basically I'd like something that guarantees that |T()| actually initializes somehow -- feels too easy to get a T that isn't one of those here.
Comment on attachment 8974444 [details] [diff] [review]
bug1460341-remove-gc-policy-initial

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

I'm going to wimp out here and punt the review over to Waldo. I'm not confident enough in my understanding of the various C++ initialization rules.

I'm also not sure how to add the static_assert safety Waldo is requesting -- in particular, where to put it. Do you need to have it everywhere you're templatized on some T and you construct it with T()?
Attachment #8974444 - Flags: review?(sphink) → review?(jwalden+bmo)
Comment on attachment 8974444 [details] [diff] [review]
bug1460341-remove-gc-policy-initial

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

::: js/public/GCPolicyAPI.h
@@ -73,5 @@
>  {
> -    static T initial() {
> -        return T();
> -    }
> -

How about instead of this function -- which, yeah, it's a pain to have every policy have to specify -- we add

namespace JS {

/**
 * Create a safely-initialized |T|, suitable for use as a default value in
 * situations requiring a safe but arbitrary |T| value.
 */
template<typename T>
inline T
SafelyInitialized()
{
    // This function wants to presume that |T()| -- which value-initializes a
    // |T| per C++11 [expr.type.conv]p2 -- will produce a safely-initialized,
    // safely-usable T that it can return.

    // That presumption holds for pointers, where value initialization produces
    // a null pointer.
    constexpr bool IsPointer = std::is_pointer<T>::value;

    // For classes we *assume* that if |T|'s default constructor is non-trivial
    // it'll initialize correctly.  (This is unideal, but C++ doesn't offer a
    // type trait indicating whether a class's constructor is user-defined,
    // which better approximates our desired semantics.)
    constexpr bool IsNonTriviallyDefaultConstructibleClass =
        std::is_class<T>::value &&
        !std::is_trivially_default_constructible<T>::value;

    static_assert(IsPointer || IsNonTriviallyDefaultConstructibleClass,
                  "T() must evaluate to a safely-initialized T");
    return T();
}

} // namespace JS

It isn't ideal to have to say |SafelyInitialized<T>()| in places where your patch can just say |T()|, but I think the extra verification means it's worth it.

If you can find some header that all these various uses depend upon in which to place this, then put it there.  Otherwise put it in js/public/SafelyInitialized.h and call it a day, I guess.  Not worth thinking hard about where this should go, IMO.

::: js/public/RootingAPI.h
@@ +757,5 @@
>    public:
> +    DispatchWrapper()
> +      : tracer(&JS::GCPolicy<T>::trace),
> +        storage()
> +    {}

I assume this is to avoid having to pass T() at the various places explicitly -- and maybe for the other new default constructors added? -- but I think I prefer passing SafelyInitialized<T>() explicitly, both here and in those other new-default-constructor cases.
Attachment #8974444 - Flags: review?(jwalden+bmo) → feedback+
std::is_class<T>::value probably needs to be or'd with std::is_union<T>::value so that JS::Value can be supported, and the related variable name should change.
Attached patch jsid constructor (obsolete) — Splinter Review
We could give jsid a (constexpr) constructor, very similar to JS::Value (the fromRawBits function is there for some CacheIR code).

This resulted in some new static initializers in the bindings code, I had to give PropertyInfo a constexpr constructor to fix that. It was not constexpr by default due to the bitfields..
Attachment #8974737 - Flags: review?(jcoppeard)
Attachment #8974737 - Flags: review?(bzbarsky)
Attached patch jsid constructorSplinter Review
Now with a comment, sorry for the churn.
Attachment #8974737 - Attachment is obsolete: true
Attachment #8974737 - Flags: review?(jcoppeard)
Attachment #8974737 - Flags: review?(bzbarsky)
Attachment #8974740 - Flags: review?(jcoppeard)
Attachment #8974740 - Flags: review?(bzbarsky)
Comment on attachment 8974740 [details] [diff] [review]
jsid constructor

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

That's simpler than what I had.
Attachment #8974740 - Flags: review?(jcoppeard) → review+
Attachment #8974443 - Attachment is obsolete: true
Attachment #8974443 - Flags: review?(jdemooij)
Comment on attachment 8974740 [details] [diff] [review]
jsid constructor

r=me
Attachment #8974740 - Flags: review?(bzbarsky) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f7779dbc0d
Give jsid a constructor that initializes it to a void id. r=jonco,bz
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #12)
> https://hg.mozilla.org/mozilla-central/rev/14f7779dbc0d

It looks like this had an effect on libxul section sizes: '.data' increased by 144,000 bytes and '.bss' decreased by 149,344 bytes. This is reflected in a 148,024 byte increase in libxul's size.
Hmm... We have arrays in bindings that look like this:

  static PropertyInfo sNativeProperties_propertyInfos[48];

where the PropertyInfo struct has a jsid member.  The struct used to not have a constructor at all, and neither did jsid, so the whole thing got zero-initialized like statics do and the arrays ended up in .bss.  That meant the jsids looked like string jsids with a null JSString*, but that was OK: no one examined them before they got initialized.  But now the jsids are getting inited to 0x2 instead, so have to go in .data.

I just checked with bloaty and we have about 140KB of .data in dom/bindings and all of it is various sNativeProperties_propertyInfos symbols.

I've been thinking of ways to make that stuff smaller anyway, but in the meantime, would it make any sense to make the JSID_VOID tag be 0 instead of 2?
Flags: needinfo?(jdemooij)
Something to be aware of: even with a value of 0, and constexpr constructors, MSVC can still decide to emit static initializers. In that case the data /might/ be in an equivalent of bss (not sure that's actually a thing in PE), but then you have useless code that actively writes 0s to the memory location on startup.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> but in the
> meantime, would it make any sense to make the JSID_VOID tag be 0 instead of
> 2?

Yeah, I'll try to make JSID_VOID == 0 today and see if that helps.
Another option is to add an UninitializedId type and use that in the bindings code, maybe that's the safest option (in terms of no memory/perf overhead). I'll see if that works out.
Depends on: 1464036
(In reply to Mike Hommey [:glandium] from comment #15)
> Something to be aware of: even with a value of 0, and constexpr
> constructors, MSVC can still decide to emit static initializers. In that
> case the data /might/ be in an equivalent of bss (not sure that's actually a
> thing in PE), but then you have useless code that actively writes 0s to the
> memory location on startup.

Is there an easy way to find out? Bug 1464036 fixes the issue on Linux but if I understand you correctly, maybe that's not sufficient on Windows and then we really want to leave these fields uninitialized.
Flags: needinfo?(jdemooij) → needinfo?(mh+mozilla)
Updated patch based on your suggestions.
Attachment #8974444 - Attachment is obsolete: true
Attachment #8980338 - Flags: review?(jwalden+bmo)
Comment on attachment 8980338 [details] [diff] [review]
bug1460341-remove-gc-policy-initial v2

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

::: js/public/RootingAPI.h
@@ +221,5 @@
> +    // doesn't offer a type trait indicating whether a class's constructor is
> +    // user-defined, which better approximates our desired semantics.)
> +    constexpr bool IsNonTriviallyDefaultConstructibleClassOrUnion =
> +        (std::is_class<T>::value || std::is_union<T>::value) &&
> +        !std::is_trivially_default_constructible<T>::value;

I discovered recently that std::is_trivially_constructible doesn't exist on a few ancient toolchains we use on treeherder.

https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/js/src/gc/Statistics.cpp#723

It wouldn't surprise me if the same applies to this trait as well.  Be sure to give those two jobs a try before pushing this as-is.  rs=me on adding a similar #if here if necessary.

Also -- let me know if this *does* work, because if so, I should change that code to use this narrower trait instead of the #if there.
Attachment #8980338 - Flags: review?(jwalden+bmo) → review+
(In reply to Jan de Mooij [:jandem] from comment #18)
> (In reply to Mike Hommey [:glandium] from comment #15)
> > Something to be aware of: even with a value of 0, and constexpr
> > constructors, MSVC can still decide to emit static initializers. In that
> > case the data /might/ be in an equivalent of bss (not sure that's actually a
> > thing in PE), but then you have useless code that actively writes 0s to the
> > memory location on startup.
> 
> Is there an easy way to find out? Bug 1464036 fixes the issue on Linux but
> if I understand you correctly, maybe that's not sufficient on Windows and
> then we really want to leave these fields uninitialized.

I don't know if there's an easy way to find out. I found out the hard way in bug 1460838, and after some fiddling, I figured I could see whether there's a static initializer by attaching the msvc debugger to firefox, pausing, opening the source file with the static variable, highlight the line, right click and select "Go to disassembly". If there is one, the disassembly for the static initializer is shown, otherwise, it displays an error. Maybe dmajor knows something better that could be automated.
Flags: needinfo?(mh+mozilla) → needinfo?(dmajor)
For a quick inspection you could do something like this in windbg. This is mozglue from the latest Nightly.

0:000> x mozglue!*dynamic*initializer*for*
00000001`80001060 mozglue!mozilla::sse_private::`dynamic initializer for 'avx2_enabled'' (void)
00000001`80001260 mozglue!mozilla::sse_private::`dynamic initializer for 'sse4_2_enabled'' (void)
00000001`80001000 mozglue!std::`dynamic initializer for '_Fac_tidy_reg'' (void)
00000001`80001160 mozglue!mozilla::sse_private::`dynamic initializer for 'mmx_enabled'' (void)
00000001`80001210 mozglue!mozilla::sse_private::`dynamic initializer for 'sse4_1_enabled'' (void)
00000001`800013d0 mozglue!`dynamic initializer for 'StaticScopeInvoke'' (void)
00000001`800013f0 mozglue!`dynamic initializer for 'sStackWalkSuppressions'' (void)
00000001`800013c0 mozglue!`dynamic initializer for 'gRecycledSize'' (void)
00000001`800011b0 mozglue!mozilla::sse_private::`dynamic initializer for 'sse3_enabled'' (void)
00000001`80001400 mozglue!mozilla::`dynamic initializer for 'sInitOnce'' (void)
00000001`80001310 mozglue!mozilla::sse_private::`dynamic initializer for 'ssse3_enabled'' (void)
00000001`800010f0 mozglue!mozilla::sse_private::`dynamic initializer for 'avx_enabled'' (void)
00000001`800012b0 mozglue!mozilla::sse_private::`dynamic initializer for 'sse4a_enabled'' (void)
00000001`80001010 mozglue!mozilla::sse_private::`dynamic initializer for 'aes_enabled'' (void)
00000001`80001360 mozglue!`dynamic initializer for 'Kernel32Intercept'' (void)
00000001`80001390 mozglue!`dynamic initializer for 'NtDllIntercept'' (void)

If you need something even more automated, you could probably coax ShowGlobals into listing such functions: https://cs.chromium.org/chromium/src/tools/win/ShowGlobals/ShowGlobals.cc

I'm not convinced there's cause for alarm with MSVC in general. The case in question with mozglue yesterday was on a --disable-optimize build. Personally I'd file that under "you got what you asked for."
Flags: needinfo?(dmajor)
(In reply to David Major [:dmajor] from comment #22)
> I'm not convinced there's cause for alarm with MSVC in general. The case in
> question with mozglue yesterday was on a --disable-optimize build.
> Personally I'd file that under "you got what you asked for."

MSVC *was* generating a static initializer for gInitLock on an optimized build.
> 00000001`800013f0 mozglue!`dynamic initializer for 'sStackWalkSuppressions'' (void)
https://searchfox.org/mozilla-central/search?q=sStackWalkSuppressions

> 00000001`800013c0 mozglue!`dynamic initializer for 'gRecycledSize'' (void)
https://searchfox.org/mozilla-central/search?q=gRecycledSize

OK, I retract the last paragraph of my previous comment.
Also...
00000001`800013c0 mozglue!`dynamic initializer for 'gRecycledSize'' (void)
wtf.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3dc9765360b
Replace GCPolicy<T>::initial() with SafelyInitialized<T>() r=Waldo
(Moving the static initializer discussion to bug 1464036...)
https://hg.mozilla.org/mozilla-central/rev/f3dc9765360b
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.