On Wed, 3 Apr 2024 16:38:13 GMT, Vladimir Kozlov <[email protected]> wrote:
>> src/hotspot/share/code/codeCache.cpp line 1009:
>>
>>> 1007: int CodeCache::nmethod_count() {
>>> 1008: int count = 0;
>>> 1009: for (GrowableArrayIterator<CodeHeap*> heap =
>>> _nmethod_heaps->begin(); heap != _nmethod_heaps->end(); ++heap) {
>>
>> Is there a reason why FOR_ALL_NMETHOD_HEAPS wasn't good fit here? I'm
>> wondering since the similar `CodeCache::blob_count()` still uses one of
>> these macros.
>
> No, `CodeCache::blob_count()` uses different macro `FOR_ALL_HEAPS(heap)`
> because it looks for all code blobs, not only nmethods.
>
> `CodeCache::nmethod_count()` is the only place where `FOR_ALL_NMETHOD_HEAPS `
> was used. So I decided to remove the macro.
I didn't say that blob_count used `FOR_ALL_NMETHODS_HEAP`. I wrote "one of
these macros". I still think this adds an inconsistency to the code that I
don't think is beneficial.
With that said, can't this be written as:
for (CodeHeap* heap : *_nmethod_heaps) {
Maybe yet another opportunity for cleanups.
>> src/hotspot/share/code/nmethod.cpp line 812:
>>
>>> 810: // By calling this nmethod entry barrier, it plays along and acts
>>> 811: // like any other nmethod found on the stack of a thread (fewer
>>> surprises).
>>> 812: nmethod* nm = as_nmethod_or_null();
>>
>> Calling as_nmethod_or_null() from within functions in the nmethod class is
>> suspicious. Shouldn't all such usages be removed? (I'm fine with doing that
>> as a separate change)
>
> Good catch! The code was moved from CompiledMethod where it made sense but
> now it is not needed. Here the change I will make:
>
> // like any other nmethod found on the stack of a thread (fewer
> surprises).
> - nmethod* nm = as_nmethod_or_null();
> - if (nm != nullptr && bs_nm->is_armed(nm)) {
> + nmethod* nm = this;
> + if (bs_nm->is_armed(nm)) {
> bool alive = bs_nm->nmethod_entry_barrier(nm);
Sounds good.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550216628
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550217712
