RFR: 8356173: Remove ThreadCritical [v4]

[Kim Barrett]

On Mon, 12 May 2025 12:05:17 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> The initialization lines are similar one-liners.
>> Deferred<> access already had the initialized check built-in, so don't need the explicit assert.
>> Deferred<> avoids the pointer indirection.
>> Deferred<> avoids dynamic allocation.
>
> I still fail to see the point of this.  dynamic allocation isn't something that's a problem for this object.  It may be for the NMT types that use this wrapper but not for this object.  You cannot write a microbenchmark to show the effect of this pointer indirection in this code path.  If you could, there's something seriously wrong with our algorithms which should be fixed instead.  The initialization check is an obvious one liner, not executable in production.
> 
> The DeferredWhat? wrapper is extra typing, that requires you to go to some other place to figure out that among all the casting, it's doing a placement new.  For no reason other than to give you more code to navigate to.

Clearly we disagree about the utility of Deferred. I won't insist on it being
used here, even though I think it's a fine use-case. I think this comment from
the PR best describes the motivation:
https://github.com/openjdk/jdk/pull/24689#issuecomment-2823556707
Unfortunately, that comment is kind of buried down in the middle of the PR.

But some of the criticisms seem incorrect. For example, using Deferred is less
typing, even without including the assert against multiple initialization that
is missing from the current code. (That's one of the problems with
boiler-plate - it's easy to forget or mess up some of it.) And I don't know
what you mean by "all the casting", as there is precisely one (const) cast in
Deferred. Maybe you are remembering earlier versions from the PR that added
it? (Some versions did have lots of casts.)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25072#discussion_r2085653262