replace the createNull method with a public static const s_null data member

[bpeabody]

This change has been discussed with the bde team, and I had intended to make it before handing Datum off to them, but never got around to it. It would simplify use of Datum, provide a potential performance benefit (as null objects are created frequently), and as a side-benefit, allow the logic in the body of createNull to move into bdld_datum.cpp.

Note that createNull could be left intact (perhaps returning a const Datum&) for compatibility.

I'll be happy to write the patch myself if this change is approved.

[RMGiroux]

Hi Brock!

Would this open up any order-of-initialization issues for statics?

We might want the s_null field to be a static local in a static function, initialized in a BCEMT_ONCE_DO just to avoid that possibility.

This sounds like a good idea, but I think we should wait for @mversche or Atilla to comment since they know more about Datum than I do.

Thanks,

Mike

[bpeabody]

Hi Mike!

That's a quick response! I don't think it would cause any ordering problems -- createNull doesn't call any methods:

    Datum result;
#ifdef BSLS_PLATFORM_CPU_32_BIT
    // Setting exponent using half-word is faster, maybe the compiler folds the
    // two statements into one?

    result.d_as.d_exponent = k_DOUBLE_MASK | Datum::e_INTERNAL_EXTENDED;
    result.d_as.d_ushort   = e_EXTENDED_INTERNAL_NIL;
#else   // BSLS_PLATFORM_CPU_32_BIT
    result.d_as.d_type     = e_INTERNAL_NIL;
#endif  // BSLS_PLATFORM_CPU_32_BIT

    return result;

But yes, definitely happy to hear the opinions of @mversche and Attila.

[dharesign]

Why would a static data member be more efficient? I would imagine you should just make createNull constexpr.

[dharesign]

https://godbolt.org/g/Exe45D

[RMGiroux]

@dharesign, not all of our compilers support constexpr yet.

If they did, things would be much simpler, I agree.

(also, nice to see another member of the large godbolt.org fan club :) )

[bpeabody]

Yeah, godbolt.org is awesome. The constexpr solution works for me, though the static seems clear and does work for your older compilers.

Re. constexpr: what other methods could it be applied to? Might it be a reasonable optimization for the other create methods? I think it's pretty common to call them with, e.g., numeric constants.

[dharesign]

I'm not sure just making createNull constexpr is valid. Some compilers were complaining about a variety of things. It probably makes more sense to provide a constexpr constructor which initializes Datum to the null state?

See below for the various options. In each case, compiling with --std=c++14 -O3:

• Current Code
  • Clang 4: https://godbolt.org/g/1aNb4q
  • GCC 6.3: https://godbolt.org/g/0zIrVB
  • GCC (snapshot): https://godbolt.org/g/Px9AaE
• s_null
  • Clang 4: https://godbolt.org/g/SwFguq
  • GCC 6.3: https://godbolt.org/g/rGVORi
  • GCC (snapshot): https://godbolt.org/g/lWyJWD
• constexpr Datum()
  • Clang 4: https://godbolt.org/g/LGEy9e
  • GCC 6.3: https://godbolt.org/g/daAtIb
  • GCC (snapshot): https://godbolt.org/g/MrEnrk

Based on this, I'm not sure creating a static s_null is really beneficial.

A liberal sprinkling of constexpr where it makes sense looks like it would provide real benefits though.

[bpeabody]

"A liberal sprinkling of constexpr where it makes sense looks like it would provide real benefits though."

Yeah, I think that is more generally useful than making a static s_null.

[dharesign]

Also note that I forgot to define createNull inline. If you do that, it's actually better than my constexpr constructor as the current implementation doesn't initialize the memory to 0.

[RMGiroux]

constexpr is great when the compiler supports it, but as I said, we have to support compilers that don't... If you do want to use it, use BSLS_CPP11_CONSTEXPR from the bsls_cpp11 component.

I still like the s_null idea for the non-cpp11 case.

[dharesign]

Let me summarize my ramblings: there is no benefit from s_null. As createNull is inline, you're not going to get much better than not initializing a block of memory and then copying a 2 into a short. Having a static s_null actually means you have to copy 16 bytes, rather than just 2 bytes.

Adding constexpr doesn't really help in the general case. If you wanted to be able to create Datum objects in constexpr contexts, then createNull etc. could be made constexpr, but to be able to do that, Datum's constructor would need to be explicitly defined and constexpr, which would lead to Datum no longer being a PODType. It would still be TriviallyCopyable though, and arguably the default constructor giving you a Datum in the null state is better than what you currently get.

[bpeabody]

There are plenty of situations where a const Datum& is expected and no copy would happen at all. I can't see how it would be more efficient to create and return a value than to use a value that is already initialized.

[dharesign]

I guess I misunderstood your use case, given:

as null objects are created frequently

If you have a need for lots of const Datum & to null Datums, then perhaps you should have your own? Sure, BDE could provide it. But maybe someone wants an s_zero, or s_emptyString. Where do you draw the line, and is this something that should be provided by Datum itself?

[bpeabody]

That's a good point, and part of the reason I was interested in constexpr in general. I think the main compelling reason I could have for s_null in particular is that it would remove the need for createNull and is unambiguously simpler and more efficient. Whereas, s_zero and s_emptyString would be special cases, extending the interface of Datum.

[dharesign]

If you actually want to create a Datum having the null state, then as I showed above using the current form of createNull is better than copying an s_null. So I don't think it could replace createNull.

And given that there would be no difference, from a performance standpoint, between an s_null Datum provided by BDE, and one provided by yourself in your application, I'm not so convinced.

[bpeabody]

Frequently, you don't need to create a new object at all. In that case, the static version is far more efficient (this is true if you use constexpr too): https://godbolt.org/g/il7qmQ

[osubboo]

Closing stale issues