ICU-11276 Using atomics for thread safety in NumberRangeFormatter.

[sffc]

I need to use some form of an atomic for proper thread safety. Here is one way to do it, my preferred option, which involves including in a public header file. If this is problematic, I can work around by newing an internal object for the atomic, which comes at the cost of extra mallocs.

Checklist

• Issue filed at https://unicode-org.atlassian.net : ICU-11276
• Update PR title to include Issue number
• Issue accepted
• Tests included
• Documentation is changed or added

[jefgen]

I don't know if we can include STL headers in the public ICU headers.

[aheninger]

That is a good question. I'm not sure of the answer. I'm a little wary too.

It would be nice to not have the dependency from the public ICU header, but getting rid of it would require some trickiness on the implementation side. I'd like to hear what everyone else thinks. The header should be standard, and the functions we need don't throw exceptions, or allocate memory, or do other dicey things - they should pretty much inline to simple machine instructions.

[sffc]

The reason I need the import here is because I need the type information.

Whether or not I #import in the public header file, I'm going to be using it internally for the implementation, something we already do elsewhere in ICU (for example, in decimfmt.cpp).

[sffc]

Was there a concern at one point that putting this in the public header means that ICU and the client have to both be compiled with the same version of STL?

Like, if the definition of std::atomic changes between C++ compiler implementations, we still want ICU to work. Maybe std::atomic is stable and this isn't actually a problem.

[aheninger]

From our public ICU headers, dragging in somewhat arcane system headers that are unrelated to our public, user-accessible API just doesn't seem desirable.

I'm guessing we could probably get away with it, but it would be nicer to avoid it. And there's some increased risk of platform problems. Although I can't immediately think of why there should be, problems have a way of happening anyhow.

[jefgen]

So from asking some folks internally, their opinion was that using std::atomic is in the same category (or just as bad) as using something like std::vector or std::string in a public header.

Their reasoning is that there can be (and have been) ABI breaking changes between library versions. For example the sizeof(std::atomic<T*>) can vary depending on the version of the library that you are building/compiling with.

This is still a problem even for private members, as the sizeof the entire class depends on the particular implementation details of std::atomic. This means that the size of the class can change if the library version changes.

So if anybody downloads pre-built binaries for ICU, and then builds with a different version of the STL library, things might not work and/or they would be broken. :(

(As an aside: supposedly there is some work in C++20 to try and address this... However, it feels like it will be long time time before we can depend on C++20 in ICU.)

[jefgen]

On second thought, perhaps this isn't as much of an issue as initially suggested and/or worried about?

From looking at the docs, it doesn't look like ICU4C makes any sort of claim about binary compatibility for C++ at all (only for the C API surface).

From the ICU docs: http://userguide.icu-project.org/design#TOC-ICU-Binary-Compatibility:-Using-ICU-as-an-Operating-System-Level-Library

Here are the requirements for enabling binary compatibility for ICU4C:

• Applications must use only APIs that are marked as stable.
• Applications must use only plain C APIs, never C++.
• ICU must be built with function renaming disabled.
• Applications must be built using an ICU that was configured for binary compatibility.
• Use ICU version 3.0 or later.
  ...

C APIs only. Only plain C APIs remain compatible across ICU releases. The reason C++ binary compatibility is not supported is primarily because the design of C++ language and runtime environments present extreme technical difficulties to doing so. Stable C++ APIs are source compatible, but applications using them must be recompiled when moving between ICU releases.

If there is only source compatibility for the C++ API surface, then this might(?) be fine, as there should be no direct usage/reference to std::atomic in the C API surface.
(Though the library DLLs like icuuc and icuin would all need to be compiled with the exact same compiler/runtime/library/etc.)

[aheninger]

Yes, there is only source compatibility for the C++ APIs, and the ICU library and the application must be built with run-time compatible compilers & libraries.

There should be no problem with the plain C APIs, no matter which way we go on this question.

[srl295]

So if anybody downloads pre-built binaries for ICU, and then builds with a different version of the STL library, things might not work and/or they would be broken. :(

This seems like it's still an issue, is this addressed?

[srl295]

Can we ifdef this to allow either an inline or out of line implementation?

…

On Mon, Sep 17, 2018 at 6:21 PM Andy Heninger ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In icu4c/source/i18n/unicode/numberrangeformatter.h <#130 (comment)>: > @@ -5,6 +5,7 @@ #ifndef __NUMBERRANGEFORMATTER_H__ #define __NUMBERRANGEFORMATTER_H__ +#include <atomic> That is a good question. I'm not sure of the answer. I'm a little wary too. It would be nice to not have the dependency from the public ICU header, but getting rid of it would require some trickiness on the implementation side. I'd like to hear what everyone else thinks. The header should be standard, and the functions we need don't throw exceptions, or allocate memory, or do other dicey things - they should pretty much inline to simple machine instructions. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#130 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0Ms6Z4WCC7PJd-iQAEyAkO8dkkxxzbks5ucEqJgaJpZM4Ws4qp> .

[srl295]

Changes LGTM as they are, but with a suggestion for avoiding atomic in the public header.

[srl295]

So if anybody downloads pre-built binaries for ICU, and then builds with a different version of the STL library, things might not work and/or they would be broken. :(

This seems like it's still an issue, is this addressed?

[srl295]

okay, I don't see fAtomicFormatter called from an inline environment. So I don't see a much of an argument for including this in a public header. Can the atomicity be implemented entirely within the .cpp side? Perhaps with an opaque type here in the class definition?

[sffc]

Yes, it can. But how to you make an opaque type? We don't know how big the std::atomic needs to be.

[aheninger]

I'm coming to the conclusion that we should go ahead and use std::atomic directly. The alternatives all add complexity or performance hits, or both.

Headers aside, if there are runtime libraries with incompatible implementations of atomics, and ICU ends up dynamically (at load time) linked with an incompatible RT library, ICU is most likely going to fail. Because of a mixture of inline and out-of-line code. This will happen no matter what choice we make for the immediate question.

But failures are, I think, quite unlikely. The language standard encourages, but does not require, implementations to keep the sizeof (std::atomic) the same as sizeof(T). For simple pointer types, with all known CPUs having direct support, it's likely true. (Less likely to be the case for larger or more complex types)

So, my vote is to try it, and if we run into problems, do something else in a maintenance fix.

[srl295]

@aheninger

…This will happen no matter what choice we make for the immediate question

your logic is good here. 👍

[srl295]

👍