Atomic value #26918
Atomic value #26918
Conversation
|
👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@kimbarrett The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
build
| using Base::relaxed_store; | ||
| using Base::release_store; | ||
| using Base::release_store_fence; | ||
| using Base::cmpxchg; |
There was a problem hiding this comment.
I waffled back and forth about these long lists of using declarations. Instead maybe macroize?
| } | ||
|
|
||
| template<typename Dummy = int, | ||
| std::enable_if_t<AtomicValueHasExchange<Decayed>::value, Dummy> = 0> |
There was a problem hiding this comment.
I have a macro elsewhere to package this little dance for non-dependent
SFINAE, a la ENABLE_IF but dealing with the lack of a dependency. The comment
describing ENABLE_IF talks about this "uncommon" case and not bothering to
provide a macro to support it, but I've run into it several times recently.
If we could rely on the availability of the __COUNTER__ extension, ENABLE_IF
could just deal with it.
| // | ||
| // static member functions: | ||
| // value_offset_in_bytes() -> int | ||
| // value_size_in_bytes() -> int |
There was a problem hiding this comment.
These return int for consistency with other related code.
| // | ||
| // static member functions: | ||
| // value_offset_in_bytes() -> int | ||
| // value_size_in_bytes() -> int |
There was a problem hiding this comment.
value_size_in_bytes() should be constexpr. Fixed in my local repository.
| // destructor | ||
| // | ||
| // static member functions: | ||
| // value_offset_in_bytes() -> int |
There was a problem hiding this comment.
value_offset_in_bytes should be constexpr, but that requires resolution of JDK-8300080.
I said I might work on an atomic value class, to encapsulate volatile + Atomic
usage. So here's a proposal. I'm showing it to a small number of folks first
in case anyone hates it and wants something completely different (possibly
including continuing with more or less what we currently have). Of course, I'm
putting this on github as a draft PR, so somene else might notice it. I'm not
worried about that.
The approach taken here is that there's AtomicValue, which is an alias
template. It selects an internal implementation class to use, based on T. A
different approach would be to expose those implementation classes and have
clients use them directly. I chose not to do that for a couple of reasons.
They'd need good names, and their names would likely be longer, so more
verbosity. We might change / restructure things over time, and having the
implementation type selected for the client makes clients insensitive to such
changes. Also, letting clients chose the implementation type would require
more error checking to ensure the implementation type and the value type are
consistent. With automatic selection I get to skip all that.
One of the things this approach has that I think is essential is that it is
fairly easy to be incremental about updating. Also, I'm expecting there will
be some cases that just shouldn't be AtomicValues, but should continue to use
Atomic operations directly.
I thought I was going to want some C++17 features for this, so I have that
turned on in the repo where I'm doing this work. It turned out I didn't take
the approach I'd been thinking would need that though. But I expect there are
a few C++17-isms that have crept in anyway. At minimum, there are uses of the
variable form of some type traits. I haven't gotten around to removing the
C++17 enabling and seeing what breaks, but should probably do that.
[later] OK, I lied. I realized there was a different way to calculate the
AtomicValue implementation type from T that leans heavily on C++17. In
particular, it relies on
if constexprand Mandatory Copy Elision. Bothmethods are present in the current draft PR, though of course only one should
be kept. But which one? Oh, and I finally learned why
static_assert(false)doesn't work sometimes. And there's a DR for that, but not all the compiler
versions we use or otherwise care about have implemented that DR.
Some of the function names I'm proposing for AtomicValue differ from the
Atomic names. Without the explicit Atomic:: prefix, I found some names a
little too subtle when looking at some updated examples. So I have
load => load_relaxed
store => relaxed_store
add => add_then_fetch (goes with existing Atomic::fetch_then_add)
sub => sub_then_fetch (goes with missing Atomic::fetch_then_sub)
inc => atomic_inc
dec => atomic_dec
I also added a couple of operations to the pointer case, things I've seen
synthesized in multiple places. These involve cmpxchg or xchg with nullptr
arguments. Though I've also figured out how to change things to eliminate the
casts we currently need when passing nullptr to those functions. That's not
part of this proposal though. (There's a C++20 type_trait that showed me what
to do, and we could easily add our own version of it.)
I'm expecting some bike-shedding on names, including AtomicValue.
I've included a couple of uses, to demonstrate things are working. Obviosly
real tests are needed. I haven't put effort in that direction, wanting to
first see whether this is a direction we want to take.
Pointer values look to often be messy. We'll also need to do something about
the APIs for things like LockFreeStack, which currently expect the element
class to provide a function that returns a pointer to volatile next member,
and would need to return a pointer to an AtomicValue with this new scheme. I
can probably figure out a way to detect which one is being provided and adapt
accordingly, so that we don't have to fix all users of such classes in one go.
We don't currently have Atomic::fetch_and_sub, which is odd. But I guess we
haven't needed it? Or have been working around the lack, rather than doing the
work to properly provide it?
We don't currently have Atomic::xchg for 1-byte values. We do have
Atomic::XchgUsingCmpxchg, so it should be pretty easy to provide a default
implementation. We can specialize for x86 (and others?) if we really want. I
suspect the amount of code involved to provide a default implementation is
comparable to the amount needed to work around that lack in AtomicValue. And I
even noticed a couple of use-cases (currently using CAS) while looking for
places to modify for demonstration purposes.
Some of these variables are known to assembly code. I've provided some helpers
for that. Some of these variables are known to VMStruct (and perhaps
JVMCIStruct, but I don't know if we care about that). I'm not sure what to do
about that.
Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26918/head:pull/26918$ git checkout pull/26918Update a local copy of the PR:
$ git checkout pull/26918$ git pull https://git.openjdk.org/jdk.git pull/26918/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26918View PR using the GUI difftool:
$ git pr show -t 26918Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26918.diff