On Tue, 30 Jun 2026 22:53:42 GMT, Serguei Spitsyn <[email protected]> wrote:
> This update is for Valhalla pre-integration. It introduces new JVMTI > capability `can_support_value_objects`. > The variable `_can_support_value_objects_count` is introduced for > optimization. It follows the pattern of the `can_support_virtual_threads` > capability. > > Additionally, this update includes test fixes: > - test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp: > - Removed the `if (!jni->HasIdentity(object))` check because it is not > needed anymore as the JVMTI capability `can_support_value_objects` is no > acquired by the test > > - These two tests are updated to provide both positive and negative coverage > for new capability: > test/hotspot/jtreg/serviceability/jvmti/valhalla/VMObjectAllocValue > test/hotspot/jtreg/serviceability/jvmti/valhalla/SampledObjectAllocValue > > Testing: > - Ran updated tests locally > - Submitted mach5 tiers 1-6 > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). I reviewed the implementation and tests only, assuming that CSR will be approved. The only general comment is that implementation doesn't check the '--enable-preview' so agent with enabled 'can_support_value_objects' is not going to fail. Do we need a separate test for this? Also, there is a general lack of coverage of this behaviour for multi environments/agents. Might be needed to to file RFE to review './vmTestbase/nsk/jvmti/scenarios/multienv' and see if they should be updated or write new tests. There are few comments inline. src/hotspot/share/prims/jvmtiExport.cpp line 2980: > 2978: EVT_TRACE(JVMTI_EVENT_VM_OBJECT_ALLOC, ("[%s] Evt vmobject alloc > sent %s", > 2979: > JvmtiTrace::safe_get_thread_name(thread), > 2980: object==nullptr? "null" : > object->klass()->external_name())); Seems the 2980 line was incorrect even without the fix. The object can't be null. See line 2964. src/hotspot/share/prims/jvmtiExport.cpp line 3023: > 3021: JvmtiEnvThreadStateIterator it(state); > 3022: for (JvmtiEnvThreadState* ets = it.first(); ets != nullptr; ets = > it.next(ets)) { > 3023: JvmtiEnv *env = ets->get_env(); Not related to you fix just general comment for next clean up. It seems that the 'ets' is needed only to get 'env'. So all loop might be rewritten similar to same in the `void JvmtiExport::post_vm_object_alloc(JavaThread *thread, oop object) {` to be `for (JvmtiEnv* env = it.first(); env != nullptr; env = it.next(env)) {` test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp line 532: > 530: jvmtiError err; > 531: > 532: if (!jni->HasIdentity(object)) { I wonder if it makes sense to update HepMontior tests to work with value classes events enabled? Not needed to implement in this fix, just file RFE for this. ------------- Marked as reviewed by lmesnik (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2607#pullrequestreview-4604754353 PR Review Comment: https://git.openjdk.org/valhalla/pull/2607#discussion_r3502431770 PR Review Comment: https://git.openjdk.org/valhalla/pull/2607#discussion_r3502476090 PR Review Comment: https://git.openjdk.org/valhalla/pull/2607#discussion_r3502409262
