RFR: 8252406: Introduce Thread::as_Java_thread() convenience function [v2]
David Holmes
david.holmes at oracle.com
Tue Sep 8 07:00:27 UTC 2020
On 8/09/2020 4:34 pm, Kim Barrett wrote:
>> On Sep 8, 2020, at 12:09 AM, David Holmes <dholmes at openjdk.java.net> wrote:
>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Used static_cast instead of raw C-style cast
>> Moved inline functions to thread.inline.hpp
>> Updated #includes to deal with the move to thread.inline.hpp
>> Updated copyright year where needed.
>>
>> -------------
>>
>> Changes:
>> - all: https://git.openjdk.java.net/jdk/pull/37/files
>> - new: https://git.openjdk.java.net/jdk/pull/37/files/b18faad5..da70f804
>>
>> Webrevs:
>> - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=01
>> - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=00-01
>>
>> Stats: 102 lines in 33 files changed: 39 ins; 28 del; 35 mod
>> Patch: https://git.openjdk.java.net/jdk/pull/37.diff
>> Fetch: git fetch https://git.openjdk.java.net/jdk pull/37/head:pull/37
>>
>> PR: https://git.openjdk.java.net/jdk/pull/37
>
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/thread.hpp
> 507 const JavaThread* as_const_Java_thread() const;
>
> This missed part of what I'd suggested. I don't think the "const" part
> of the name is needed or helpful. Just overload as_Java_thread on the
> const-ness of the thread it's being applied to.
I saw that but thought it was a typo as I don't think about overloads in
terms of const or other modifiers. To me overloads are same named
methods that differ in arguments (and I'm going to guess that 'this' is
considered an argument in this context and hence leads to the overload
based on the const?).
But in addition I wanted to have this say as_const_JavaThread because I
wanted to clearly distringuish it from as_JavaThread because there is
only a couple of special cases in the JFR code where this const version
is needed. If that code were to change and is_const_JavaThread because
unused, it would be easy to see that.
> There are some situations where there is such an overload pair and it
> is useful to additionally have an explicit const function, but I don't
> think this is one of those.
>
> ------------------------------------------------------------------------------
>
> The fannout from putting the conversion functions in thread.inline.hpp seems
> pretty high in the number of files that get touched. Especially since it
> leads to JavaThread::current() and JavaThread::as_CompilerThread() being
> moved. I would have been at least fine with, and probably would prefer,
> having the definitions of as_JavaThread near the old definitions of those
> functions in thread.hpp.
I tried to do the right and obey the "use .inline.hpp for inline
definitions" rule. But I must confess I was reaching a point where I
thought this was going too far but it's a good exercise with using git
and PRs so I don't mind if I back up and take a simpler approach -
especially as I just discovered more build failures due to missing
includes. :(
Thanks,
David
> ------------------------------------------------------------------------------
>
More information about the serviceability-dev
mailing list