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
Backport-of: 8f5f44070a7c6dbbbd1005f9d0af5ab7c35179df
  • Loading branch information
Hannes Greule authored and shipilev committed Oct 26, 2023
1 parent 5ebc02b commit c5a6a74
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 @@ -37,6 +37,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 @@ -48,7 +49,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 @@ -1096,7 +1096,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 @@ -1108,7 +1108,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 @@ -1142,7 +1142,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 @@ -1176,7 +1176,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 @@ -1188,7 +1188,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 @@ -1200,7 +1200,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);
}

}

1 comment on commit c5a6a74

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.