Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8286066: assert(k != __null) failed: klass not loaded caused by FillerObject_klass #8519

Closed
wants to merge 11 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented May 3, 2022

Hi all,

FillerObject_klass was loaded too late, which may lead to VM crash due to FillerObject_klass is unloaded.
We found this bug by running runtime/cds/appcds/TestEpsilonGCWithCDS.java on x86_32.

From the following gdb backtrace, FillerObject_klass may be used during vmClasses::resolve_all (see frame #27 resolving Exception_klass).
So FillerObject_klass should be loaded as soon as possible.

(gdb) bt
#0  0xf66c5506 in vmClasses::check_klass (k=0x0) at /home/jdk/src/hotspot/share/classfile/vmClasses.hpp:55
#1  0xf68de7a7 in vmClasses::FillerObject_klass () at /home/jdk/src/hotspot/share/classfile/vmClasses.hpp:87
#2  0xf695ce9e in CollectedHeap::fill_with_object_impl (start=0xb2e370c8, words=2, zap=true) at /home/jdk/src/hotspot/share/gc/shared/collectedHeap.cpp:470
#3  0xf695b40b in CollectedHeap::fill_with_object (start=0xb2e370c8, words=2, zap=true) at /home/jdk/src/hotspot/share/gc/shared/collectedHeap.cpp:479
#4  0xf695bf4e in CollectedHeap::fill_with_object (start=0xb2e370c8, end=0xb2e370d0, zap=true) at /home/jdk/src/hotspot/share/gc/shared/collectedHeap.hpp:289
#5  0xf695b4fd in CollectedHeap::fill_with_dummy_object (this=0xf602c250, start=0xb2e370c8, end=0xb2e370d0, zap=true)
    at /home/jdk/src/hotspot/share/gc/shared/collectedHeap.cpp:503
#6  0xf71d5866 in ThreadLocalAllocBuffer::insert_filler (this=0xf6017cdc) at /home/jdk/src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp:122
#7  0xf71d5949 in ThreadLocalAllocBuffer::retire (this=0xf6017cdc, stats=0x0) at /home/jdk/src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp:145
#8  0xf71d5995 in ThreadLocalAllocBuffer::retire_before_allocation (this=0xf6017cdc) at /home/jdk/src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp:152
#9  0xf6ed49c0 in MemAllocator::allocate_inside_tlab_slow (this=0xf62255c0, allocation=...) at /home/jdk/src/hotspot/share/gc/shared/memAllocator.cpp:309
#10 0xf6ed48ea in MemAllocator::allocate_inside_tlab (this=0xf62255c0, allocation=...) at /home/jdk/src/hotspot/share/gc/shared/memAllocator.cpp:278
#11 0xf6ed4b47 in MemAllocator::mem_allocate (this=0xf62255c0, allocation=...) at /home/jdk/src/hotspot/share/gc/shared/memAllocator.cpp:350
#12 0xf6ed4b9e in MemAllocator::allocate (this=0xf62255c0) at /home/jdk/src/hotspot/share/gc/shared/memAllocator.cpp:363
#13 0xf6c1b199 in CollectedHeap::class_allocate (this=0xf602c250, klass=0xaa41bca8, size=26, __the_thread__=0xf6017c10)
    at /home/jdk/src/hotspot/share/gc/shared/collectedHeap.inline.hpp:46
#14 0xf6c1aead in InstanceMirrorKlass::allocate_instance (this=0xaa41bca8, k=0xaa429d70, __the_thread__=0xf6017c10) at /home/jdk/src/hotspot/share/oops/instanceMirrorKlass.cpp:55
#15 0xf6c396b5 in java_lang_Class::create_mirror (k=0xaa429d70, class_loader=..., module=..., protection_domain=..., classData=..., __the_thread__=0xf6017c10)
    at /home/jdk/src/hotspot/share/classfile/javaClasses.cpp:1004
#16 0xf690c541 in ClassFileParser::fill_instance_klass (this=0xf62257d4, ik=0xaa429d70, changed_by_loadhook=false, cl_inst_info=..., __the_thread__=0xf6017c10)
    at /home/jdk/src/hotspot/share/classfile/classFileParser.cpp:5428


    ....


#27 0xf724e3ea in vmClasses::resolve (id=vmClassID::Exception_klass_knum, __the_thread__=0xf6017c10) at /home/jdk/src/hotspot/share/classfile/vmClasses.cpp:99


#28 0xf724e4ed in vmClasses::resolve_until (limit_id=vmClassID::SoftReference_klass_knum, start_id=@0xf6225fa8: vmClassID::Cloneable_klass_knum, __the_thread__=0xf6017c10)
    at /home/jdk/src/hotspot/share/classfile/vmClasses.cpp:108
#29 0xf724ef3f in vmClasses::resolve_through (last_id=vmClassID::Reference_klass_knum, start_id=@0xf6225fa8: vmClassID::Cloneable_klass_knum, __the_thread__=0xf6017c10)
    at /home/jdk/src/hotspot/share/classfile/vmClasses.hpp:64
#30 0xf724e75e in vmClasses::resolve_all (__the_thread__=0xf6017c10) at /home/jdk/src/hotspot/share/classfile/vmClasses.cpp:168
#31 0xf718845a in SystemDictionary::initialize (__the_thread__=0xf6017c10) at /home/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1655
#32 0xf71f9814 in Universe::genesis (__the_thread__=0xf6017c10) at /home/jdk/src/hotspot/share/memory/universe.cpp:346
#33 0xf71fb851 in universe2_init () at /home/jdk/src/hotspot/share/memory/universe.cpp:966
#34 0xf6c07275 in init_globals () at /home/jdk/src/hotspot/share/runtime/init.cpp:132
#35 0xf71cc844 in Threads::create_vm (args=0xf622626c, canTryAgain=0xf62261d3) at /home/jdk/src/hotspot/share/runtime/thread.cpp:2756

The fix just loads FillerObject after Object_klass.

Thanks.
Best regards,
Jie


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 reviews required, with at least 1 reviewer)

Issue

  • JDK-8286066: assert(k != __null) failed: klass not loaded caused by FillerObject_klass

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8519/head:pull/8519
$ git checkout pull/8519

Update a local copy of the PR:
$ git checkout pull/8519
$ git pull https://git.openjdk.java.net/jdk pull/8519/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8519

View PR using the GUI difftool:
$ git pr show -t 8519

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8519.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 3, 2022

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label May 3, 2022
@openjdk
Copy link

openjdk bot commented May 3, 2022

@DamonFool The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime label May 3, 2022
@DamonFool
Copy link
Member Author

DamonFool commented May 3, 2022

/label hotspot-gc

@DamonFool
Copy link
Member Author

DamonFool commented May 3, 2022

/label add hotspot-gc

@openjdk openjdk bot added the hotspot-gc label May 3, 2022
@openjdk
Copy link

openjdk bot commented May 3, 2022

@DamonFool
The hotspot-gc label was successfully added.

@openjdk
Copy link

openjdk bot commented May 3, 2022

@DamonFool The hotspot-gc label was already applied.

@mlbridge
Copy link

mlbridge bot commented May 3, 2022

/* GC support, should be loaded as early as possible */ \
do_klass(FillerObject_klass, jdk_internal_vm_FillerObject ) \
Copy link
Member

@dholmes-ora dholmes-ora May 3, 2022

Choose a reason for hiding this comment

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

That seems just a touch too early for my liking, Even though it is an empty class it is in a different package and that might perturb a few things in an unexpected way.

IIUC the class is going to be needed when the initial TLAB is expanded. Did you determine exactly which class was causing that? I'd like to see this loading delayed until after the core system classes, as much as possible.

Thanks.

Copy link
Member Author

@DamonFool DamonFool May 4, 2022

Choose a reason for hiding this comment

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

That seems just a touch too early for my liking, Even though it is an empty class it is in a different package and that might perturb a few things in an unexpected way.

IIUC the class is going to be needed when the initial TLAB is expanded. Did you determine exactly which class was causing that? I'd like to see this loading delayed until after the core system classes, as much as possible.

Thanks.

Thanks @dholmes-ora for the review.

I added a jtreg to this bug.
It can also be reproduced with java -XX:-UseCompressedClassPointers -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC -Xshare:dump on Linux/x64.

I saw the crash after java.lang.ThreadDeath was loaded on x86_32.
Maybe, we can load FillerObject after java.lang.Error.

STDERR:
 stdout: [[0.004s][info][class,path] bootstrap loader class path=/home/jdk/build/linux-x86-server-fastdebug/images/jdk/lib/modules
[0.006s][warning][gc,init   ] Consider enabling -XX:+AlwaysPreTouch to avoid memory commit hiccups
[0.006s][info   ][cds       ] Core region alignment: 4096
[0.006s][info   ][class,path] app loader class path=/home/jdk/build/linux-x86-server-fastdebug/test-support/jtreg_test_hotspot_jtreg_runtime_cds_appcds_TestEpsilonGCWithCDS_java/scratch/0/hello.jar
[0.006s][info   ][class,path] opened: /home/jdk/build/linux-x86-server-fastdebug/test-support/jtreg_test_hotspot_jtreg_runtime_cds_appcds_TestEpsilonGCWithCDS_java/scratch/0/hello.jar
[0.006s][info   ][class,load] opened: /home/jdk/build/linux-x86-server-fastdebug/test-support/jtreg_test_hotspot_jtreg_runtime_cds_appcds_TestEpsilonGCWithCDS_java/scratch/0/hello.jar
[0.010s][info   ][class,load] java.lang.Object source: jrt:/java.base
[0.010s][info   ][class,load] java.io.Serializable source: jrt:/java.base
[0.010s][info   ][class,load] java.lang.Comparable source: jrt:/java.base
[0.010s][info   ][class,load] java.lang.CharSequence source: jrt:/java.base
[0.010s][info   ][class,load] java.lang.constant.Constable source: jrt:/java.base
[0.010s][info   ][class,load] java.lang.constant.ConstantDesc source: jrt:/java.base
[0.011s][info   ][class,load] java.lang.String source: jrt:/java.base
[0.011s][info   ][class,load] java.lang.reflect.AnnotatedElement source: jrt:/java.base
[0.012s][info   ][class,load] java.lang.reflect.GenericDeclaration source: jrt:/java.base
[0.012s][info   ][class,load] java.lang.reflect.Type source: jrt:/java.base
[0.012s][info   ][class,load] java.lang.invoke.TypeDescriptor source: jrt:/java.base
[0.012s][info   ][class,load] java.lang.invoke.TypeDescriptor$OfField source: jrt:/java.base
[0.012s][info   ][class,load] java.lang.Class source: jrt:/java.base
[0.012s][info   ][class,load] java.lang.Cloneable source: jrt:/java.base
[0.013s][info   ][class,load] java.lang.ClassLoader source: jrt:/java.base
[0.013s][info   ][class,load] java.lang.System source: jrt:/java.base
[0.013s][info   ][class,load] java.lang.Throwable source: jrt:/java.base
[0.013s][info   ][class,load] java.lang.Error source: jrt:/java.base
[0.013s][info   ][class,load] java.lang.ThreadDeath source: jrt:/java.base
# To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc:  SuppressErrorAt=/vmClasses.hpp:55
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/home/jdk/src/hotspot/share/classfile/vmClasses.hpp:55), pid=78368, tid=78371
#  assert(k != __null) failed: klass not loaded
#
# JRE version:  (19.0) (fastdebug build )
# Java VM: OpenJDK Server VM (fastdebug 19-internal-adhoc..jdk, interpreted mode, tiered, epsilon gc, linux-x86)
# Problematic frame:
# V  [libjvm.so+0x85f54d]  CollectedHeap::fill_with_dummy_object(HeapWordImpl**, HeapWordImpl**, bool)+0x42d
#

What do you think?
Thanks.

Copy link
Member Author

@DamonFool DamonFool May 4, 2022

Choose a reason for hiding this comment

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

Maybe, we can load FillerObject after java.lang.Error.

But I'm not sure if it's always safe to do so on any platforms.

Copy link
Member

@dholmes-ora dholmes-ora May 4, 2022

Choose a reason for hiding this comment

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

For the x64 case (thanks for that reproducer!) it crashes after loading java.lang.Thread$UncaughtExceptionHandler - so somewhat later than the x86 case.
I thought the underlying problem was the TLAB size but at least with EpsilonGC it is acting very counterintuitively - any size > 6K crashes, while less does not. But logging also seems to show that the requested size is not being honoured in those cases but the Ergo size of 2K is being selected! This is all very odd, but perhaps specific to Epsilon.
With G1 I can crash with a TLABSize of 4K (or less), but it occurs much later after loading jdk.internal.reflect.UnsafeStaticFieldAccessorImpl (for 4K). With 2K minimum TLABSize it is after java.lang.Boolean.
So I guess after Error is a reasonable compromise.
Thanks.

Copy link
Member Author

@DamonFool DamonFool May 4, 2022

Choose a reason for hiding this comment

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

So I guess after Error is a reasonable compromise.

Updated.
runtime/cds/appcds/TestEpsilonGCWithCDS.java runtime/cds/FillerObjectLoadTest.java passed on Linux/x86_32.
More testing is in progress.
Thanks.

Copy link
Member Author

@DamonFool DamonFool May 4, 2022

Choose a reason for hiding this comment

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

With G1 I can crash with a TLABSize of 4K (or less), but it occurs much later after loading

The jtreg test had been updated to also test the default GC.
Thanks.

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm.

@DamonFool
Copy link
Member Author

DamonFool commented May 4, 2022

I tested tier1~tier3 on Linux/x64, no regression.
Thanks.

@openjdk
Copy link

openjdk bot commented May 4, 2022

@DamonFool This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8286066: assert(k != __null) failed: klass not loaded caused by FillerObject_klass

Reviewed-by: dholmes, tschatzl, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 25 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 4, 2022
@tschatzl
Copy link
Contributor

tschatzl commented May 4, 2022

An alternative fix which seems more comprehensive could be to use java_lang_Object() for filling these gaps while the other isn't initialized.

E.g. in

CollectedHeap::fill_with_object_impl(HeapWord* start, size_t words, bool zap)
{
  assert(words <= filler_array_max_size(), "too big for a single object");

  if (words >= filler_array_min_size()) {
    fill_with_array(start, words, zap);
  } else if (words > 0) {
    assert(words == min_fill_size(), "unaligned size");
    ObjAllocator allocator(vmClasses::FillerObject_klass(), words);
    allocator.initialize(start);
  }
}

instead of direct use of vmClasses::FillerObject_klass(), add a helper function that selects it based on some predicate; not sure if Universe::is_initialized() (or so) isn't too lenient.

I see that vmClasses already has some XXXX_class_loaded() methods - maybe add one of these for the filler objects and select on that?

Use of the small filler objects is really rare, additionally it only happens at most once per TLAB/PLAB fill, and is mostly done by the application anyway, so I do not see an issue with such an additional flag.

Another option is to have some global variable containing that klass (either in Universe or maybe better in CollectedHeap) that initially aliases java_lang_Object and after this class loading is complete, set it to the filler object klass (that is then used by CollectedHeap::fill_with_object_impl.

This would completely avoid the somewhat brittle guessing about the initialization order of the klasses, and avoids any runtime overhead by checking whether the klass has already been loaded during runtime at the cost of a single global variable. At this time I kind of prefer this second option.

Copy link
Contributor

@tschatzl tschatzl left a comment

Changing my approval after thinking it through a bit more to discuss the options we have.

@openjdk openjdk bot removed the ready label May 4, 2022
@DamonFool
Copy link
Member Author

DamonFool commented May 4, 2022

An alternative fix which seems more comprehensive could be to use java_lang_Object() for filling these gaps while the other isn't initialized.

E.g. in

CollectedHeap::fill_with_object_impl(HeapWord* start, size_t words, bool zap)
{
  assert(words <= filler_array_max_size(), "too big for a single object");

  if (words >= filler_array_min_size()) {
    fill_with_array(start, words, zap);
  } else if (words > 0) {
    assert(words == min_fill_size(), "unaligned size");
    ObjAllocator allocator(vmClasses::FillerObject_klass(), words);
    allocator.initialize(start);
  }
}

instead of direct use of vmClasses::FillerObject_klass(), add a helper function that selects it based on some predicate; not sure if Universe::is_initialized() (or so) isn't too lenient.

I see that vmClasses already has some XXXX_class_loaded() methods - maybe add one of these for the filler objects and select on that?

Use of the small filler objects is really rare, additionally it only happens at most once per TLAB/PLAB fill, and is mostly done by the application anyway, so I do not see an issue with such an additional flag.

Another option is to have some global variable containing that klass (either in Universe or maybe better in CollectedHeap) that initially aliases java_lang_Object and after this class loading is complete, set it to the filler object klass (that is then used by CollectedHeap::fill_with_object_impl.

This would completely avoid the somewhat brittle guessing about the initialization order of the klasses, and avoids any runtime overhead by checking whether the klass has already been loaded during runtime at the cost of a single global variable. At this time I kind of prefer this second option.

Thanks @tschatzl for your review and suggestion.

Then part of the unused memory would be filled with Object and the others would be filled with FillerObject.
I'm not sure if this kind of change is safe and if it would complicate the design/implementation/optimization of HotSpot GC algorithms.
What do you think of my worries?
Thanks.

@tschatzl
Copy link
Contributor

tschatzl commented May 4, 2022

Then part of the unused memory would be filled with Object and the others would be filled with FillerObject. I'm not sure if this kind of change is safe and if it would complicate the design/implementation/optimization of HotSpot GC algorithms. What do you think of my worries? Thanks.

The gc filler objects only provide additional verification and easier debugging (i.e. any reference to such a filler object is simply an error somewhere). We don't (and can't) verify that every non-FillerObject is being referenced at all times.

Also, the "wrong" filler objects would also only persist until the first garbage collection, as

  • all Universe initialization needs to happen before that (the VM will fail otherwise)
  • (g1 only) all objects allocated during initialization are (for g1) non-humongous
  • evacuation will reclaim these wrong fillers (and a potential evacuation failure will re-fill the holes with the correct fillers)

This can certainly be combined with early as possible initialization of that global variable with the correct j.i.v.FillerObject to reduce the number of these objects.

So I believe this is safe (and actually necessary to avoid specific TLAB sizes to fail anyway) to do.

Thanks,
Thomas

@DamonFool
Copy link
Member Author

DamonFool commented May 4, 2022

Then part of the unused memory would be filled with Object and the others would be filled with FillerObject. I'm not sure if this kind of change is safe and if it would complicate the design/implementation/optimization of HotSpot GC algorithms. What do you think of my worries? Thanks.

The gc filler objects only provide additional verification and easier debugging (i.e. any reference to such a filler object is simply an error somewhere). We don't (and can't) verify that every non-FillerObject is being referenced at all times.

Also, the "wrong" filler objects would also only persist until the first garbage collection, as

  • all Universe initialization needs to happen before that (the VM will fail otherwise)
  • (g1 only) all objects allocated during initialization are (for g1) non-humongous
  • evacuation will reclaim these wrong fillers (and a potential evacuation failure will re-fill the holes with the correct fillers)

This can certainly be combined with early as possible initialization of that global variable with the correct j.i.v.FillerObject to reduce the number of these objects.

So I believe this is safe (and actually necessary to avoid specific TLAB sizes to fail anyway) to do.

Thanks, Thomas

Sounds good!
Updated.
Thanks.

@iklam
Copy link
Member

iklam commented May 4, 2022

If we decide to go with the latest implementation, the bug title should be updated.

String java_home_src = System.getProperty("java.home");
String java_home_dst = CDSTestUtils.getOutputDir() + File.separator + "moved_jdk";
CDSTestUtils.clone(new File(java_home_src), new File(java_home_dst));
String dstJava = java_home_dst + File.separator + "bin" + File.separator + "java";
Copy link
Member

@dholmes-ora dholmes-ora May 4, 2022

Choose a reason for hiding this comment

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

Can't you avoid all this by specifying the name of the shared archive file for the dump?

Copy link
Member Author

@DamonFool DamonFool May 4, 2022

Choose a reason for hiding this comment

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

Can't you avoid all this by specifying the name of the shared archive file for the dump?

It works!
Updated.
Thanks.

@DamonFool DamonFool changed the title 8286066: FillerObject_klass should be loaded as early as possible 8286066: assert(k != __null) failed: klass not loaded caused by FillerObject_klass May 4, 2022
@DamonFool
Copy link
Member Author

DamonFool commented May 4, 2022

If we decide to go with the latest implementation, the bug title should be updated.

Updated.
Thanks.

ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
"-XX:+IgnoreUnrecognizedVMOptions", "-XX:-UseCompressedClassPointers",
"-XX:+UnlockExperimentalVMOptions", "-XX:+UseEpsilonGC", "-Xshare:dump",
"-XX:SharedArchiveFile=./hello.jsa");
Copy link
Member

@dholmes-ora dholmes-ora May 5, 2022

Choose a reason for hiding this comment

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

Use TestCommon.getCurrentArchiveName() to get a unique name to use.

Copy link
Member

@dholmes-ora dholmes-ora May 5, 2022

Choose a reason for hiding this comment

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

Actually that might be overkill for this simple case ... I wonder if @iklam could comment?

hello.jsa is a bit of a weird name to use though :)

Copy link
Member Author

@DamonFool DamonFool May 5, 2022

Choose a reason for hiding this comment

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

Use TestCommon.getCurrentArchiveName() to get a unique name to use.

Hi @dholmes-ora , I didn't get the point why we need to get a unique name.
What's the problem if we always use the same name?
Thanks.

Copy link
Member

@dholmes-ora dholmes-ora May 5, 2022

Choose a reason for hiding this comment

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

We had issues in the past with tests running concurrently IIRC . I'm not sure if this is still an issue if the test working directory is already unique. @iklam should remember the details. We put in a lot of support code to make sure things always worked smoothly.

Copy link
Member

@iklam iklam May 5, 2022

Choose a reason for hiding this comment

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

We had problems on Windows in the past. If two tests generate an archive with the same name, after the first test finishes, jtreg deletes the first version of this file. However, Windows virus scanners will keep the file alive for a little longer. When the second tests tries to create the file again, it gets a file permission error. This would lead to infrequent random failures in the CDS tests.

That's the reason we use TestCommon.getCurrentArchiveName() in the CDS tests.

Copy link
Member

@iklam iklam May 5, 2022

Choose a reason for hiding this comment

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

We had issues in the past with tests running concurrently IIRC . I'm not sure if this is still an issue if the test working directory is already unique.

The archive file is generated in the test's "working scratch" directory, which is reused. For example, if you have 3 test cases and a jtreg concurrency setting of 2: 0/scratch/ could be used twice whereas 1/scratch/ is used once.

Copy link
Member Author

@DamonFool DamonFool May 5, 2022

Choose a reason for hiding this comment

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

We had problems on Windows in the past. If two tests generate an archive with the same name, after the first test finishes, jtreg deletes the first version of this file. However, Windows virus scanners will keep the file alive for a little longer. When the second tests tries to create the file again, it gets a file permission error. This would lead to infrequent random failures in the CDS tests.

That's the reason we use TestCommon.getCurrentArchiveName() in the CDS tests.

Thanks @iklam and @dholmes-ora for your kind explanation.

Updated.
TestCommon.getCurrentArchiveName() returns null so I use TestCommon.getNewArchiveName() instead.
Thanks.

iklam
iklam approved these changes May 5, 2022
Copy link
Member

@iklam iklam left a comment

LGTM.

@openjdk openjdk bot added the ready label May 5, 2022
@@ -60,6 +60,7 @@

class ClassLoaderData;

Klass* CollectedHeap::_filler_klass = NULL;
Copy link
Contributor

@tschatzl tschatzl May 5, 2022

Choose a reason for hiding this comment

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

Please name this member _filler_object_klass to better distinguish it from _filler_array*.

Copy link
Member Author

@DamonFool DamonFool May 5, 2022

Choose a reason for hiding this comment

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

Please name this member _filler_object_klass to better distinguish it from _filler_array*.

Updated.
Thanks.

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm. Thanks.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Looks good! But I'd like to test it in our CI before integration please.

Thanks,
David

@tschatzl
Copy link
Contributor

tschatzl commented May 5, 2022

I'll push it through our tier1-3 and report back

@dholmes-ora
Copy link
Member

dholmes-ora commented May 5, 2022

I had a clear run on tier1-3 so all good to integrate

@DamonFool
Copy link
Member Author

DamonFool commented May 5, 2022

Thank you all for the review and testing.
/integrate

@openjdk
Copy link

openjdk bot commented May 5, 2022

Going to push as commit 7ebc4bc.
Since your change was applied there have been 41 commits pushed to the master branch:

  • 6a1b145: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
  • 59ef76a: 8285497: Add system property for Java SE specification maintenance version
  • 6d7e446: 8283306: re-resolving indirect call to non-entrant nmethod can crash
  • 4957bc7: 8286056: AArch64: clarify uses of MacroAssembler::far_call/MacroAssembler::far_jump
  • e7adc28: 8284675: "jpackage.exe" creates application launcher without Windows Application Manfiest
  • 9644a31: 8285616: [macos] Incorrect path for launcher-as-service.txt in .cfg file
  • 2f995c8: 8286199: ProblemList jdk/jshell/ExternalEditorTest.java
  • 2293448: 8272352: Java launcher can not parse Chinese character when system locale is set to UTF-8
  • 1bba640: 8284027: vmTestbase/nsk/jvmti/GetAllThreads/allthr001/ is failing
  • 5ac7186: 8282559: Allow multiple search terms in javadoc search
  • ... and 31 more: https://git.openjdk.java.net/jdk/compare/efcd3d3a8fd19909ef21a07cc61613aad4dbe145...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label May 5, 2022
@openjdk openjdk bot closed this May 5, 2022
@openjdk openjdk bot removed ready rfr labels May 5, 2022
@openjdk
Copy link

openjdk bot commented May 5, 2022

@DamonFool Pushed as commit 7ebc4bc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@DamonFool DamonFool deleted the JDK-8286066 branch May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-runtime integrated
4 participants