Skip to content

Commit

Permalink
8317692: jcmd GC.heap_dump performance regression after JDK-8292818
Browse files Browse the repository at this point in the history
Reviewed-by: amenkov, fparain
  • Loading branch information
Hannes Greule authored and David Holmes committed Oct 19, 2023
1 parent 684b91e commit 8f5f440
Show file tree
Hide file tree
Showing 3 changed files with 299 additions and 8 deletions.
103 changes: 102 additions & 1 deletion src/hotspot/share/oops/fieldStreams.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
// iterates over fields that have been injected by the JVM.
// AllFieldStream exposes all fields and should only be used in rare
// cases.
// HierarchicalFieldStream allows to also iterate over fields of supertypes.
class FieldStreamBase : public StackObj {
protected:
const Array<u1>* _fieldinfo_stream;
Expand Down Expand Up @@ -135,7 +136,7 @@ class FieldStreamBase : public StackObj {
}
};

// Iterate over only the internal fields
// Iterate over only the Java fields
class JavaFieldStream : public FieldStreamBase {
public:
JavaFieldStream(const InstanceKlass* k): FieldStreamBase(k->fieldinfo_stream(), k->constants(), 0, k->java_fields_count()) {}
Expand Down Expand Up @@ -179,4 +180,104 @@ class AllFieldStream : public FieldStreamBase {
AllFieldStream(const InstanceKlass* k): FieldStreamBase(k->fieldinfo_stream(), k->constants()) {}
};

// Iterate over fields including the ones declared in supertypes
template<typename FieldStreamType>
class HierarchicalFieldStream : public StackObj {
private:
const Array<InstanceKlass*>* _interfaces;
InstanceKlass* _next_klass; // null indicates no more type to visit
FieldStreamType _current_stream;
int _interface_index;

void prepare() {
_next_klass = next_klass_with_fields();
// special case: the initial klass has no fields. If any supertype has any fields, use that directly.
// if no such supertype exists, done() will return false already.
next_stream_if_done();
}

InstanceKlass* next_klass_with_fields() {
assert(_next_klass != nullptr, "reached end of types already");
InstanceKlass* result = _next_klass;
do {
if (!result->is_interface() && result->super() != nullptr) {
result = result->java_super();
} else if (_interface_index > 0) {
result = _interfaces->at(--_interface_index);
} else {
return nullptr; // we did not find any more supertypes with fields
}
} while (FieldStreamType(result).done());
return result;
}

// sets _current_stream to the next if the current is done and any more is available
void next_stream_if_done() {
if (_next_klass != nullptr && _current_stream.done()) {
_current_stream = FieldStreamType(_next_klass);
assert(!_current_stream.done(), "created empty stream");
_next_klass = next_klass_with_fields();
}
}

public:
HierarchicalFieldStream(InstanceKlass* klass) :
_interfaces(klass->transitive_interfaces()),
_next_klass(klass),
_current_stream(FieldStreamType(klass)),
_interface_index(_interfaces->length()) {
prepare();
}

void next() {
_current_stream.next();
next_stream_if_done();
}

bool done() const { return _next_klass == nullptr && _current_stream.done(); }

// bridge functions from FieldStreamBase

AccessFlags access_flags() const {
return _current_stream.access_flags();
}

FieldInfo::FieldFlags field_flags() const {
return _current_stream.field_flags();
}

Symbol* name() const {
return _current_stream.name();
}

Symbol* signature() const {
return _current_stream.signature();
}

Symbol* generic_signature() const {
return _current_stream.generic_signature();
}

int offset() const {
return _current_stream.offset();
}

bool is_contended() const {
return _current_stream.is_contended();
}

int contended_group() const {
return _current_stream.contended_group();
}

FieldInfo to_FieldInfo() {
return _current_stream.to_FieldInfo();
}

fieldDescriptor& field_descriptor() const {
return _current_stream.field_descriptor();
}

};

#endif // SHARE_OOPS_FIELDSTREAMS_HPP
14 changes: 7 additions & 7 deletions src/hotspot/share/services/heapDumper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "memory/allocation.inline.hpp"
#include "memory/resourceArea.hpp"
#include "memory/universe.hpp"
#include "oops/fieldStreams.inline.hpp"
#include "oops/klass.inline.hpp"
#include "oops/objArrayKlass.hpp"
#include "oops/objArrayOop.inline.hpp"
Expand All @@ -50,7 +51,6 @@
#include "runtime/javaThread.inline.hpp"
#include "runtime/jniHandles.hpp"
#include "runtime/os.hpp"
#include "runtime/reflectionUtils.hpp"
#include "runtime/threads.hpp"
#include "runtime/threadSMR.hpp"
#include "runtime/vframe.hpp"
Expand Down Expand Up @@ -935,7 +935,7 @@ u4 DumperSupport::instance_size(Klass* k) {
InstanceKlass* ik = InstanceKlass::cast(k);
u4 size = 0;

for (FieldStream fld(ik, false, false); !fld.eos(); fld.next()) {
for (HierarchicalFieldStream<JavaFieldStream> fld(ik); !fld.done(); fld.next()) {
if (!fld.access_flags().is_static()) {
size += sig2size(fld.signature());
}
Expand All @@ -947,7 +947,7 @@ u4 DumperSupport::get_static_fields_size(InstanceKlass* ik, u2& field_count) {
field_count = 0;
u4 size = 0;

for (FieldStream fldc(ik, true, true); !fldc.eos(); fldc.next()) {
for (JavaFieldStream fldc(ik); !fldc.done(); fldc.next()) {
if (fldc.access_flags().is_static()) {
field_count++;
size += sig2size(fldc.signature());
Expand Down Expand Up @@ -981,7 +981,7 @@ void DumperSupport::dump_static_fields(AbstractDumpWriter* writer, Klass* k) {
InstanceKlass* ik = InstanceKlass::cast(k);

// dump the field descriptors and raw values
for (FieldStream fld(ik, true, true); !fld.eos(); fld.next()) {
for (JavaFieldStream fld(ik); !fld.done(); fld.next()) {
if (fld.access_flags().is_static()) {
Symbol* sig = fld.signature();

Expand Down Expand Up @@ -1015,7 +1015,7 @@ void DumperSupport::dump_static_fields(AbstractDumpWriter* writer, Klass* k) {
void DumperSupport::dump_instance_fields(AbstractDumpWriter* writer, oop o) {
InstanceKlass* ik = InstanceKlass::cast(o->klass());

for (FieldStream fld(ik, false, false); !fld.eos(); fld.next()) {
for (HierarchicalFieldStream<JavaFieldStream> fld(ik); !fld.done(); fld.next()) {
if (!fld.access_flags().is_static()) {
Symbol* sig = fld.signature();
dump_field_value(writer, sig->char_at(0), o, fld.offset());
Expand All @@ -1027,7 +1027,7 @@ void DumperSupport::dump_instance_fields(AbstractDumpWriter* writer, oop o) {
u2 DumperSupport::get_instance_fields_count(InstanceKlass* ik) {
u2 field_count = 0;

for (FieldStream fldc(ik, true, true); !fldc.eos(); fldc.next()) {
for (JavaFieldStream fldc(ik); !fldc.done(); fldc.next()) {
if (!fldc.access_flags().is_static()) field_count++;
}

Expand All @@ -1039,7 +1039,7 @@ void DumperSupport::dump_instance_field_descriptors(AbstractDumpWriter* writer,
InstanceKlass* ik = InstanceKlass::cast(k);

// dump the field descriptors
for (FieldStream fld(ik, true, true); !fld.eos(); fld.next()) {
for (JavaFieldStream fld(ik); !fld.done(); fld.next()) {
if (!fld.access_flags().is_static()) {
Symbol* sig = fld.signature();

Expand Down
190 changes: 190 additions & 0 deletions test/hotspot/jtreg/serviceability/HeapDump/FieldsInInstanceTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

import java.io.File;
import java.lang.ref.Reference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.TimeUnit;

import jdk.test.lib.Asserts;
import jdk.test.lib.JDKToolLauncher;
import jdk.test.lib.apps.LingeredApp;
import jdk.test.lib.process.ProcessTools;
import jdk.test.lib.hprof.model.JavaClass;
import jdk.test.lib.hprof.model.JavaHeapObject;
import jdk.test.lib.hprof.model.JavaObject;
import jdk.test.lib.hprof.model.JavaThing;
import jdk.test.lib.hprof.model.Snapshot;
import jdk.test.lib.hprof.parser.Reader;

/*
* @test
* @bug 8317692
* @summary Verifies heap dump contains all fields of an instance
* @library /test/lib
* @run driver FieldsInInstanceTest
*/
class FieldsInInstanceTarg extends LingeredApp {

public static void main(String[] args) {
B b = new B();
NoFields2 nf = new NoFields2();
NoParentFields npf = new NoParentFields();
OnlyParentFields opf = new OnlyParentFields();
DirectParentNoFields dpnf = new DirectParentNoFields();
LingeredApp.main(args);
Reference.reachabilityFence(b);
Reference.reachabilityFence(nf);
Reference.reachabilityFence(npf);
Reference.reachabilityFence(opf);
Reference.reachabilityFence(dpnf);
}

interface I {
int i = -10;
}
static abstract class A implements I {
static boolean b;
int a = 3;
String s = "Field";
}
static class B extends A {
static String f = null;
int a = 7;
double s = 0.5d;
}

// no fields:
interface I1 {
}
static class NoFields1 {
}
static class NoFields2 extends NoFields1 implements I1 {
}

// no parent fields
static class NoParentFields extends NoFields1 implements I1 {
int i1 = 1;
int i2 = 2;
}

// only parent fields
static class Parent1 {
int i3 = 3;
}
static class OnlyParentFields extends Parent1 {
}

// in between parent with no fields
static class DirectParentNoFields extends OnlyParentFields {
int i = 17;
}
}

public class FieldsInInstanceTest {

public static void main(String[] args) throws Exception {
File dumpFile = new File("Myheapdump.hprof");
createDump(dumpFile, args);
verifyDump(dumpFile);
}

private static void createDump(File dumpFile, String[] extraOptions) throws Exception {
LingeredApp theApp = null;
try {
theApp = new FieldsInInstanceTarg();

List<String> extraVMArgs = new ArrayList<>();
extraVMArgs.addAll(Arrays.asList(extraOptions));
LingeredApp.startApp(theApp, extraVMArgs.toArray(new String[0]));

//jcmd <pid> GC.heap_dump <file_path>
JDKToolLauncher launcher = JDKToolLauncher
.createUsingTestJDK("jcmd")
.addToolArg(Long.toString(theApp.getPid()))
.addToolArg("GC.heap_dump")
.addToolArg(dumpFile.getAbsolutePath());
Process p = ProcessTools.startProcess("jcmd", new ProcessBuilder(launcher.getCommand()));
// If something goes wrong with heap dumping most likely we'll get crash of the target VM.
while (!p.waitFor(5, TimeUnit.SECONDS)) {
if (!theApp.getProcess().isAlive()) {
log("ERROR: target VM died, killing jcmd...");
p.destroyForcibly();
throw new Exception("Target VM died");
}
}

if (p.exitValue() != 0) {
throw new Exception("Jcmd exited with code " + p.exitValue());
}
} finally {
LingeredApp.stopApp(theApp);
}
}

private static void verifyDump(File dumpFile) throws Exception {
Asserts.assertTrue(dumpFile.exists(), "Heap dump file not found.");

log("Reading " + dumpFile + "...");
try (Snapshot snapshot = Reader.readFile(dumpFile.getPath(), true, 0)) {
log("Resolving snapshot...");
snapshot.resolve(true);
log("Snapshot resolved.");

List<JavaThing> bFields = getFields(snapshot, FieldsInInstanceTarg.B.class);
// B has 2 instance fields, A has 2 instance fields
Asserts.assertEquals(bFields.size(), 4);
// JavaObject reverses the order of fields, so fields of B are at the end.
// Order is only specified for supertypes, so we check if values are *anywhere* in their range
// by using the toString output.
String asString = bFields.subList(2, 4).toString();
Asserts.assertTrue(asString.contains("0.5"), "value for field B.s not found");
Asserts.assertTrue(asString.contains("7"), "value for field B.a not found");
asString = bFields.subList(0, 2).toString();
Asserts.assertTrue(asString.contains("3"), "value for field A.a not found");
Asserts.assertTrue(asString.contains("Field"), "value for field A.s not found");

Asserts.assertEquals(getFields(snapshot, FieldsInInstanceTarg.NoFields2.class).size(), 0);

Asserts.assertEquals(getFields(snapshot, FieldsInInstanceTarg.NoParentFields.class).size(), 2);

Asserts.assertEquals(getFields(snapshot, FieldsInInstanceTarg.OnlyParentFields.class).size(), 1);

Asserts.assertEquals(getFields(snapshot, FieldsInInstanceTarg.DirectParentNoFields.class).size(), 2);
}
}

private static List<JavaThing> getFields(Snapshot snapshot, Class<?> clazz) {
JavaObject javaObject = (JavaObject) snapshot.findClass(clazz.getName()).getInstances(false).nextElement();
List<JavaThing> fields = Arrays.asList(javaObject.getFields());
log("Fields for " + clazz + " (including superclasses): " + fields);
return fields;
}

private static void log(Object s) {
System.out.println(s);
}

}

3 comments on commit 8f5f440

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SirYwell
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/backport jdk21u

@openjdk
Copy link

@openjdk openjdk bot commented on 8f5f440 Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SirYwell the backport was successfully created on the branch SirYwell-backport-8f5f4407 in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 8f5f4407 from the openjdk/jdk repository.

The commit being backported was authored by Hannes Greule on 19 Oct 2023 and was reviewed by Alex Menkov and Frederic Parain.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git SirYwell-backport-8f5f4407:SirYwell-backport-8f5f4407
$ git checkout SirYwell-backport-8f5f4407
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git SirYwell-backport-8f5f4407

Please sign in to comment.