Eliminate redundant copies of EMPTY_UNIT object, saving 2.5 KB.

[jefgen]

Description of the changes:

There are currently 11 copies of the EMPTY_UNIT object in the
Calculator.exe binary, which not only wastes space/footprint in the
binary itself, but also means that each copy must be separately
initialized, which effects performance.

The reason for this is that the object is defined in a shared header
file, which then is included by multiple .cpp files, causing each
translation unit (.obj) to get a full complete copy of the object.

By marking the object as inline we can
instruct the linker to define the object once, as we do
not need to have 11 unique versions of the EMPTY_UNIT object,
we only need 1.

The net result is that the Calculator.exe binary size is reduced by
2,560 bytes (or 2.5 KB) with no change in behavior, other than
the small performance benefit of not initializing 10 redundant copies
of the object.

How changes were validated:

• Manually tested.

[jlaanstra]

This one could probably be constexpr.

[joshkoon]

This one could probably be constexpr.

Yes - in general, prefer standard C++ over Microsoft extensions. inline constexpr seems like a better solution.

[jlaanstra]

constexpr already implies inline, so no need to specify that separetly.

[jefgen]

Thanks for the feedback on the pull-request!

I tried making it constexpr, but unfortunately, the Unit class uses std::wstring which doesn't currently support constexpr constuction it seems. :(

I end up with various errors that are all similar to the following:

constexpr constructor calls non-constexpr function "std::basic_string<_Elem, _Traits, Alloc>::basic_string() [with _Elem=wchar_t, _Traits=std::char_traits<wchar_t>, _Alloc=std::allocator<wchar_t>]"

It looks like there is some work/proposals (link) in the C++ community to make std::string and friends constexpr, but so far nothing yet.

[joshkoon]

I tried making it constexpr, but unfortunately, the Unit class uses std::wstring which doesn't currently support constexpr constuction it seems. :(

It looks like there is some work/proposals (link) in the C++ community to make std::string and friends constexpr, but so far nothing yet.

std::wstring_view is what we should be using for the Unit constructors, and are compatible with constexpr. Could you update the constructors to accept wstring_views, instead?

[fwcd]

If #211 gets merged, __declspec(selectany):

https://github.com/Microsoft/calculator/blob/8983a1cb46c3d20aa41d0471385ab7a2370f6a18/src/CalcManager/UnitConverter.h#L54

would have to be replaced by the DECLSPEC_SELECTANY macro as in df3112b:

https://github.com/Microsoft/calculator/blob/df3112b54bf04ed8386780ebc87f8af981b6dd75/src/CalcManager/Header%20Files/EngineStrings.h#L222

to ensure GCC compatibility.

Edit: Using constexpr instead would of course work too

[jefgen]

std::wstring_view is what we should be using for the Unit constructors, and are compatible with constexpr. Could you update the constructors to accept wstring_views, instead?

I think the Unit class is supposed to own the strings though, otherwise what will keep the underlying strings alive alive if it just uses a std::wstring_view? (Or maybe I am missing something?)

[joshkoon]

Yep, you're right. My mistake. :)

[danbelcher-MSFT]

Hey @jefgen, I looked at your change and I think the right fix is to replace extern __declspec(selectany) with inline. In this context, inline means, essentially, if there would be multiple definitions of a variable or function marked inline, the linker should use just one definition. This is equivalent to the intended behavior with your change. I ran a diff between all three options and here are size results:

• Release build, master branch: 4,150,272 bytes
• Release build, extern __declspec(selectany): 4,147,712 bytes
• Release build, inline: 4,147,712 bytes

As you can see, the difference in size reduction is the same (Please verify for yourself).

Given that the size reduction is the same, we should prefer inline because it is standard C++ and avoids any issues later, such as using a macro for __declspec(selectany).

[jefgen]

Thanks for taking a look at the changes @danbelcher-MSFT!

I rebuilt using inline instead of extern __declspec(selectany) and I get the same results when examining the output from the linker. (Only one copy of the object instead of 11).

I've pushed an additional commit with the change. Though I'm not sure if you'd rather I force push a new commit, or perhaps just squash merge the two commits when merging?

[danbelcher-MSFT]

Don't worry about it. We're taking responsibility of updating the commit message when we close the PR out. Thanks for the consideration though!

[danbelcher-MSFT]

Thanks, Jeff!