Re: RFR: 8269186: [REDO] Remove CodeCache::mark_for_evol_deoptimization() method

[Lois Foltan]

On Wed, 23 Jun 2021 17:27:00 GMT, Coleen Phillimore  wrote:

> This is somewhat trivial change to remove 
> CodeCache::mark_for_evol_deoptimization() and its calling method, and nothing 
> else this time.
> Ran vmTestbase/nsk/jvmti tests.

LGTM.

Thanks,
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4575

Re: RFR: 8264711: More runtime TRAPS cleanups

[Lois Foltan]

On Mon, 5 Apr 2021 17:57:13 GMT, Harold Seigel  wrote:

> Please review this additional cleanup of use of TRAPS in hotspot runtime 
> code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and 
> Windows and Mach5 tiers 3-5 on Linux x64.
> 
> Thanks, Harold

Looks good Harold.  One minor comment for the file jfrJavaSupport.cpp.

Thanks,
Lois

src/hotspot/share/jfr/jni/jfrJavaSupport.cpp line 144:

> 142:   ObjectSynchronizer::jni_enter(h_obj, THREAD->as_Java_thread());
> 143:   ObjectSynchronizer::notifyall(h_obj, THREAD);
> 144:   ObjectSynchronizer::jni_exit(THREAD->as_Java_thread(), h_obj());

For consistency can you switch the parameter order for jni_enter as well in 
this change?  It looks a little bit odd that jni_exit was changed and not 
jni_enter.

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3345

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Lois Foltan]

On Wed, 31 Mar 2021 21:22:39 GMT, Coleen Phillimore  wrote:

>> src/hotspot/share/prims/jvm.cpp line 950:
>> 
>>> 948:   InstanceKlass* ik = NULL;
>>> 949:   if (!is_hidden) {
>>> 950: ClassLoadInfo cl_info(protection_domain);
>> 
>> Minor comment, you could pull the creation of ClassLoadInfo out of this if 
>> statement since both the the if and the else sections create a ClassLoadInfo 
>> with pretty much the same information.
>
> That other ClassLoadInfo cl_info(protection_domain) you see is from another 
> function, so I can't pull it out.
> The other side of the 'if' statement creates a ClassLoadInfo with all the 
> hidden class goodies.

In jvm_lookup_define_class there are 2 ClassLoadInfo cl_info constructions on 
line #950 and line #961 that could be pull out of the if statement.  Again 
comment is minor and I am ok with how you decide to go forward with it.

-

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Lois Foltan]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore  wrote:

>> This function is used to call the classfile parser for hidden or anonymous 
>> classes, and for use with jvmti RedefineClasses. The latter only calls 
>> KlassFactory::create_from_stream and skips the rest of the code in 
>> SystemDictionary::parse_stream.
>> 
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version 
>> depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
>> protection domain via. ClassLoadInfo.
>> 
>> So the external API is resolve_from_stream.
>> 
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fifix comment

Nice clean up Coleen.  One minor comment.

Thanks,
Lois

src/hotspot/share/prims/jvm.cpp line 950:

> 948:   InstanceKlass* ik = NULL;
> 949:   if (!is_hidden) {
> 950: ClassLoadInfo cl_info(protection_domain);

Minor comment, you could pull the creation of ClassLoadInfo out of this if 
statement since both the the if and the else sections create a ClassLoadInfo 
with pretty much the same information.

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264193: Remove TRAPS parameters for modules and defaultmethods

[Lois Foltan]

On Mon, 29 Mar 2021 17:40:09 GMT, Harold Seigel  wrote:

> Please review this change for JDK-8264193 to remove unneeded TRAPS parameters 
> from modules and default methods files.  Besides removing TRAPS, 
> Modules::get_named_module() was changed to return an oop instead of a 
> jobject, removing its need for a TRAPS parameter.
> 
> This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and 
> Windows, and Mach5 tiers 3-5 on Linux x64.
> 
> Thanks, Harold

Looks good Harold!
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3247

Re: RFR: 8263976: Remove block allocation from BasicHashtable

[Lois Foltan]

On Mon, 22 Mar 2021 15:49:24 GMT, Coleen Phillimore  wrote:

> From CR:
> The useful/general BasicHashtable uses a block allocation scheme to 
> reportedly reduce fragmentation. When the StringTable and SymbolTable used to 
> use this hashtable, performance benefits were reportedly observed because of 
> the block allocation scheme. Since these tables were moved to the concurrent 
> hashtables, the tables left that use the block allocation scheme are:
> 
> AdapterHandlerLibrary, ResolutionError, LoaderConstraints, Leak profiler 
> bitset table and Placeholders. 3 of these tables are very small and never 
> needed block allocation to prevent fragmentation at least. Also there are 3 
> KVHashtables, which are built from BasicHashtable. 2 are used during dumping 
> and 1 is ID2KlassTable which appears small.
> 
> ModuleEntry, PackageEntry, Dictionary, G1RootSet for nmethods, and 
> JvmtiTagMap tables didn't use the block allocation scheme.
> 
> Removing this removes 7 pointers per table, and for each ClassLoaderData, 
> which has 3 tables, removes 21 pointers.
> 
> This change was performance tested on linux and windows.
> 
> It was also tested with tier1-6.

Great clean up Coleen!  ModuleEntryTable and PackageEntryTable changes looks 
good as does the other changes as well.
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3123

Re: RFR: 8261340: Fix 'deprecated' warnings in the vmTestbase/nsk tests

[Lois Foltan]

On Mon, 8 Feb 2021 19:54:03 GMT, Harold Seigel  wrote:

> Please review this small fix for JDK-8261340 to clean up deprecation 
> warnings, such as the following, in the vmTestbase/nsk tests.
> 
> warning: [dep-ann] deprecated item is not annotated with @Deprecated
> 
> The change was tested by running the tests locally and checking for the 
> warnings.  It was regression tested with tiers 1-2 on Linux, Mac OS, and 
> Windows and tiers 3-5 on Linux x64.
> 
> Thanks, Harold

Looks good!
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2462

Re: RFR: 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests

[Lois Foltan]

On Fri, 5 Feb 2021 14:48:25 GMT, Harold Seigel  wrote:

> Please review this change to clean up warnings, such as the following, in the 
> vmTestbase tests.  
> 
> warning: [synchronization] attempt to synchronize on an instance of a 
> value-based class 
> warning: [removal] Integer(int) in Integer has been deprecated and marked for 
> removal
> 
> This change cleans up the warnings by using a non-value based class to 
> synchronize on, and replacing calls such as Integer(int) with 
> Integer.valueOf(int).
> 
> The change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and 
> Windows, and Mach5 tiers 3-8 on Linux x64.
> 
> Thanks, Harold

Looks good.
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2427

Re: RFR: 8259375: JvmtiExport::jni_Get/SetField_probe calls do not need ResetNoHandleMark

[Lois Foltan]

On Thu, 7 Jan 2021 16:04:28 GMT, Coleen Phillimore  wrote:

> This is a trivial change to remove the comments and the ResetNoHandleMark 
> from jvmti functions.  They're called by JNI_ENTRY so they don't have a 
> NoHandleMark that needs to be reset.  It may not have always been the case.
> Tested with the other patch for 
> https://bugs.openjdk.java.net/browse/JDK-8258032 and retesting with tier1-3.

Looks good & trivial.

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1975

Re: RFR: 8259067: bootclasspath append takes out object lock [v3]

[Lois Foltan]

On Wed, 6 Jan 2021 00:18:19 GMT, Coleen Phillimore  wrote:

>> See CR for details.
>> I made the classpath append list lock-free.  Calling experts in Atomic 
>> operations...
>> Tested with tier1-6.
>> Thanks,
>> Coleen
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Move locking into function where exclusive access is needed.

Changes look good to me.
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1935

Re: RFR: 8259067: bootclasspath append takes out object lock

[Lois Foltan]

On Mon, 4 Jan 2021 17:28:30 GMT, Coleen Phillimore  wrote:

> See CR for details.
> I made the classpath append list lock-free.  Calling experts in Atomic 
> operations...
> Tested with tier1-6.
> Thanks,
> Coleen

LGTM.
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1935

Re: RFR: 8257726: Make -XX:+StressLdcRewrite option a diagnostic option

[Lois Foltan]

On Tue, 15 Dec 2020 17:26:25 GMT, Coleen Phillimore  wrote:

> See bug for details.  Tested:
> 
> $ java -XX:+StressLdcRewrite -version
> Error: VM option 'StressLdcRewrite' is diagnostic and must be enabled via 
> -XX:+UnlockDiagnosticVMOptions.
> Error: The unlock option must precede 'StressLdcRewrite'.
> Error: Could not create the Java Virtual Machine.
> Error: A fatal exception has occurred. Program will exit.
> $ java -XX:+UnlockDiagnosticVMOptions -XX:+StressLdcRewrite -version
> openjdk version "16-internal" 2021-03-16
> OpenJDK Runtime Environment (build 16-internal+0-2020-12-15-1356558.coleen...)
> OpenJDK 64-Bit Server VM (build 16-internal+0-2020-12-15-1356558.coleen..., 
> mixed mode, sharing)
> 
> Also, java/lang/instrument which has a test using StressLdcRewrite and tier1.

Looks good Coleen.
Lois

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1783

Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal

[Lois Foltan]

On Mon, 7 Dec 2020 15:50:55 GMT, Harold Seigel  wrote:

>> Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100).
>> 
>> Development has been broken into 5 tasks, each with its own JBS issue:
>> - Deprecate wrapper class constructors for removal (rriggs)
>> - Revise "value-based class" & apply to wrappers (dlsmith)
>> - Define & apply annotation jdk.internal.ValueBased (rriggs)
>> - Add 'lint' warning for @ValueBased classes (sadayapalam)
>> - Diagnose synchronization on @ValueBased classes (lfoltan)
>> 
>> All changes have been previously reviewed and integrated into the [`jep390` 
>> branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` 
>> repository. See the subtasks of the 5 JBS issues for these changes, 
>> including discussion and links to reviews. (Reviewers: mchung, dlsmith, 
>> jlaskey, rriggs, lfoltan, fparain, hseigel.)
>> 
>> CSRs have also been completed or are nearly complete:
>> 
>> - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for 
>> wrapper class constructor deprecation
>> - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for 
>> revisions to "value-based class"
>> - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new 
>> `javac` lint warnings
>> 
>> Here's an overview of the files changed:
>> 
>> - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to 
>> an instance of a class tagged with `jdk.internal.ValueBased`. This enhances 
>> previous work that produced such diagnostics for the primitive wrapper 
>> classes.
>> 
>> - `src/java.base/share/classes/java/lang`: deprecating for removal the 
>> wrapper class constructors; revising the definition of "value-based class" 
>> in `ValueBased.html` and the description used by linking classes; applying 
>> "value-based class" to the primitive wrapper classes; marking value-based 
>> classes with `@jdk.internal.ValueBased`.
>> 
>> - `src/java.base/share/classes/java/lang/constant`: no longer designating 
>> these classes as "value-based", since they rely heavily on field inheritance.
>> 
>> - `src/java.base/share/classes/java/time`: revising the description used to 
>> link to `ValueBased.html`; marking value-based classes with 
>> `@jdk.internal.ValueBased`.
>> 
>> - `src/java.base/share/classes/java/util`: revising the description used to 
>> link to `ValueBased.html`; marking value-based classes with 
>> `@jdk.internal.ValueBased`.
>> 
>> - `src/java.base/share/classes/jdk/internal`: define the 
>> `@jdk.internal.ValueBased` annotation.
>> 
>> - `src/java.management.rmi`: removing uses of wrapper class constructors.
>> 
>> - `src/java.xml`: removing uses of wrapper class constructors.
>> 
>> - `src/jdk.compiler`: implementing the `synchronization` lint category, 
>> which reports attempts to synchronize on classes and interfaces annotated 
>> with `@jdk.internal.ValueBased`.
>> 
>> - `src/jdk.incubator.foreign`: revising the description used to link to 
>> `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a 
>> special module export, which was not deemed necessary for an incubating API.)
>> 
>> - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and 
>> synchronization warnings in tests
>> 
>> - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics
>> 
>> - `test`: testing new `javac` and HotSpot behavior; removing usages of 
>> deprecated wrapper class constructors from tests, or suppressing deprecation 
>> warnings; revising the description used to link to `ValueBased.html`.
>
> The hotspot changes look good.  In a future change, could you add additional 
> classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent.  
> Currently, it only tests primitive classes.

@hseigel thank you for the review.  I have created 
https://bugs.openjdk.java.net/browse/JDK-8257836 as an RFE to address 
additional testing.
Lois

-

PR: https://git.openjdk.java.net/jdk/pull/1636

Re: RFR: 8256052: Remove unused allocation type from fieldInfo [v3]

[Lois Foltan]

On Mon, 9 Nov 2020 19:54:09 GMT, Lois Foltan  wrote:

>> Frederic Parain has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove unused symbol from vmStruct
>
> Marked as reviewed by lfoltan (Reviewer).

Looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/1130

Re: RFR: 8256052: Remove unused allocation type from fieldInfo [v3]

[Lois Foltan]

On Mon, 9 Nov 2020 19:54:58 GMT, Frederic Parain  wrote:

>> Please review this small cleanup code, removing the now unused allocation 
>> type from the fieldInfo structure.
>> 
>> Tested with Mach5, tiers 1 to 3 and locally by running 
>> test/hotspot/jtreg/serviceability/sa tests.
>> 
>> Thank you,
>> 
>> Fred
>
> Frederic Parain has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove unused symbol from vmStruct

Marked as reviewed by lfoltan (Reviewer).

Looks good Fred.

-

PR: https://git.openjdk.java.net/jdk/pull/1130Marked as reviewed by lfoltan 
(Reviewer).

Re: RFR: JDK-8225056 VM support for sealed classes

[Lois Foltan]

On 5/21/2020 2:33 PM, Harold Seigel wrote:

Hi David,

Thanks for looking at this!  Please review this new webrev:

   http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/


Hi Harold,

I think this webrev looks good!  A couple of minor comments:

- oops/instanceKlass.cpp:
  line #236, do you need a ResourceMark here? I noticed there is one 
above at line #229 for the log_trace call that uses external_name().


- prims/jvmtiRedefineClasses.cpp:
  line #878, I think you need a ResourceMark for this method as well if 
you invoke the external_name for the log_trace calls and for 
NEW_RESOURCE_ARRAY_RETURN_NULL()?


Tests look good.

Thanks,
Lois



This webrev contains the following significant changes:

1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
   removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
   changes were from when sealed classes were marked as final, and are
   no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
   permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
   'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
   letters.
7. Changes to langtools tests were removed from this webrev. They are
   covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.

Please also see comments inline.


On 5/19/2020 1:26 AM, David Holmes wrote:

Hi Harold,

Adding serviceability-dev for the serviceability related changes.

Nit: "VM support for sealed classes"

This RFR covers the VM, core-libs, serviceability and even some 
langtools tests. AFAICS only the javac changes are not included here 
so when and where will they be reviewed and under what bug id? 
Ideally there will be a single JBS issue for "Implementation of JEP 
360: Sealed types". It's okay to break up the RFRs across different 
areas, but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406.  We 
understand the need to do the reviews under one bug.


Overall this looks good. I've looked at all files and mainly have 
some style nits in various places. But there are some more 
significant items below.


On 14/05/2020 7:09 am, Harold Seigel wrote:

Hi,

Please review this patch for JVM and Core-libs support for the JEP 
360 Sealed Classes preview feature.  Code changes include the 
following:


  * Processing of the new PermittedSubclasses attribute to enforce that
    a class or interface, whose super is a sealed class or interface,
    must be listed in the super's PermittedSubclasses attribute.
  * Disallow redefinition of a sealed class or interface if its
    redefinition would change its PermittedSubclasses attribute.
  * Support API's to determine if a class or interface is sealed 
and, if

    it's sealed, return a list of its permitted subclasses.
  * asm support for the PermittedSubclasses attribute


I assume Remi is providing the upstream support in ASM? :) But also 
see below ...




Open Webrev: 
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html


make/data/jdwp/jdwp.spec

There needs to be a sub-task and associated CSR request for this JDWP 
spec update. I couldn't see this covered anywhere.


---

src/hotspot/share/classfile/classFileParser.cpp

3215 u2 
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const 
ClassFileStream* const cfs,

3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {

Indention on L3216/17 needs fixing after the copy'n'edit.

3515   return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516  _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517  Arguments::enable_preview();

Too much indentation on L3516/17

3790 // Check for PermittedSubclasses tag

That comment (copied from my nestmates code :) is in the wrong place. 
It needs to be before


3788 if (tag == vmSymbols::tag_permitted_subclasses()) {


Minor nit: I would suggest checking 
parsed_permitted_subclasses_attribute before checking ACC_FINAL.


3876   if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses = 
parse_classfile_permitted_subclasses_attribute(

3878    cfs,
3879 permitted_subclasses_attribute_start,
3880    CHECK);

Although it looks odd the preceding, similarly shaped, sections all 
indent to the same absolute position. Can you make L3878/78/80 match 
please.


3882   guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884   sizeof(num_of_subclasses) + sizeof(u2) * 
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class 
file %s", CHECK);


Nits: please reformat as:

3882   guarantee_property(
3883

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Lois Foltan]

On 4/2/2020 2:56 PM, Mandy Chung wrote:

Hi David,

Thank you for checking thoroughly.   I now grep on src/hotspot and 
clean all of them.


Updated delta webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04-delta/ 



Patch looks good Mandy.
Lois



On 4/1/20 11:21 PM, David Holmes wrote:

Hi Mandy,

On 2/04/2020 3:17 pm, Mandy Chung wrote:

Hi David,

Thanks for the edits to the comments.   "weak hidden" were missed to 
be changed to "non-strong hidden".  Here is the patch fixing the 
comments.


There are 33 cases of "weak hidden" in the patch I reviewed and also 
some "hidden weak". Should they all be "non-strong hidden" now? 


yes, supposed to.   All should be fixed in webrev.04-delta.


In some cases it also appears in variables and associated logic ie.:

src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp

+  _hidden_weak_classes(NULL), _num_hidden_weak_classes(0),



Are you reading webrev.03?    This has been fixed in webrev.04.

I found Klass::is_hidden_weak that should have been renamed too.


I'm not clear how far this terminology change needs to extend ??



I hope consistently used in the source.


Otherwise patch below seems fine.



Revised the patch.

Thanks
Mandy

Thanks,
David
-


diff --git a/src/hotspot/share/ci/ciField.cpp 
b/src/hotspot/share/ci/ciField.cpp

--- a/src/hotspot/share/ci/ciField.cpp
+++ b/src/hotspot/share/ci/ciField.cpp
@@ -223,8 +223,8 @@
    holder->is_in_package("jdk/internal/foreign") || 
holder->is_in_package("jdk/incubator/foreign") ||

    holder->is_in_package("java/lang"))
  return true;
-  // Trust VM hidden and unsafe anonymous classes. They are created 
via Lookup.defineClass or
-  // the private API (jdk.internal.misc.Unsafe) and can't be 
serialized, so there is no hacking
+  // Trust hidden and VM unsafe anonymous classes. They are created 
via Lookup.defineClass or
+  // the private jdk.internal.misc.Unsafe API and can't be 
serialized, so there is no hacking

    // of finals going on with them.
    if (holder->is_hidden() || holder->is_unsafe_anonymous())
  return true;
diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp 
b/src/hotspot/share/ci/ciInstanceKlass.cpp

--- a/src/hotspot/share/ci/ciInstanceKlass.cpp
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp
@@ -76,7 +76,7 @@
    oop holder = ik->klass_holder();
    if (ik->class_loader_data()->has_class_mirror_holder()) {
  // Though ciInstanceKlass records class loader oop, it's not 
enough to keep
-    // VM weak hidden and unsafe anonymous classes alive (loader == 
NULL). Klass holder should
+    // non-strong hidden class and VM unsafe anonymous classes 
alive (loader == NULL). Klass holder should
  // be used instead. It is enough to record a ciObject, since 
cached elements are never removed
  // during ciObjectFactory lifetime. ciObjectFactory itself is 
created for
  // every compilation and lives for the whole duration of the 
compilation.
diff --git a/src/hotspot/share/classfile/classLoaderData.hpp 
b/src/hotspot/share/classfile/classLoaderData.hpp

--- a/src/hotspot/share/classfile/classLoaderData.hpp
+++ b/src/hotspot/share/classfile/classLoaderData.hpp
@@ -127,9 +127,10 @@
    bool _accumulated_modified_oops; // Mod Union Equivalent (CMS 
support)


    int _keep_alive; // if this CLD is kept alive.
-   // Used for weak hidden classes, unsafe 
anonymous classes and the
+   // Used for non-strong hidden classes, 
unsafe anonymous classes and the
 // boot class loader. _keep_alive does 
not need to be volatile or
-   // atomic since there is one unique CLD 
per weak hidden or unsafe anonymous class.
+   // atomic since there is one unique CLD 
per non-strong hidden class

+   // or unsafe anonymous class.

    volatile int _claim; // non-zero if claimed, for example during 
GC traces.
 // To avoid applying oop closure more than 
once.

@@ -242,15 +243,15 @@
    }

    // Returns true if this class loader data is for the system 
class loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong 
hidden class or unsafe anonymous class)

    bool is_system_class_loader_data() const;

    // Returns true if this class loader data is for the platform 
class loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong 
hidden class or unsafe anonymous class)

    bool is_platform_class_loader_data() const;

    // Returns true if this class loader data is for the boot class 
loader.
-  // (Note that the class loader data may be for an weak hidden 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong 
hidd

Re: RFR (T) 8241320: The ClassLoaderData::_is_unsafe_anonymous field is unused in the SA

[Lois Foltan]

Looks good.
Lois

On 3/19/2020 3:46 PM, coleen.phillim...@oracle.com wrote:
Summary: remove unused code that is changing in Hotspot for hidden 
classes.


Ran tier1-3 tests.  See bug for more details.

open webrev at http://cr.openjdk.java.net/~coleenp/2020/8241320.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8241320

Thanks,
Coleen

Re: RFR 8235360: Update JDWP, JDI and Instrumentation specs for Record attribute

[Lois Foltan]

On 12/5/2019 9:28 AM, Harold Seigel wrote:

Hi,

Please review this trivial change to add documentation about the 
Record attribute to the JDWP, JDI, and Instrumentation specs.


The changed .html pages (best viewed as 'raw') are included in the 
webrev but will not be pushed.


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235360/webrev/index.html


JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8235360

The fix was regression tested by running Mach5 tiers 1 and 2 tests and 
builds on Linux-x64, Solaris, Windows, and Mac OS X.


Thanks, Harold


Looks good & trivial.  VirtualMachine.java needs a copyright update.
Lois

Re: RFR: JEP 359-Records: hotspot runtime and serviceability code

[Lois Foltan]

On 10/18/2019 2:44 PM, Vicente Romero wrote:

Hi,

Please review the hotspot runtime and serviceability code for JEP 359 
(Records).


Thanks in advance for the feedback,
Vicente

PS, Thanks to Harold for the development


[1] 
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/


Harold, Vicente,

Overall this looks good!  Some comments:

src/hotspot/share/classfile/classFileParser.cpp
- line #3226: I mentiond this below that I would like to see the comment 
from jvmtiClassFileReconstituter.cp ahead of
ClassFileParser::parse_classfile_record_attribute() so it is easy to 
follow the expected layout.
- line #3275: Ahead of the for loop it would be good to add a comment 
listing what the expected attributes are for Records.
- line #4732: The added check of "!major_gte_14" looks incorrect for 
final abstract classes.  That isn't relevant to Records, correct?


src/hotspot/share/prims/jvm.cpp
- line #1737 - #1739: use oopFactory::new_objArrayHandle() instead of 
breaking this accross 2 lines.
- line #1751: please add a comment that indicates the behavior when the 
InstanceKlass is not a record.  It seems like an empty array is returned?


src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp
- copyright update needed.
- line #427-438: I like the comment, can you add that ahead of 
ClassFileParser::parse_classfile_record_attribute().


src/hotspot/share/prims/jvmtiRedefineClasses.cpp
- line #839 - comment "of" should be "if"?

src/hotspot/share/oops/recordComponent.hpp
- line #95 - determination of TBD comment needed before committing.

I still need to review the tests.

Thanks,
Lois

Re: RFR(XXS) 8213275 ReplaceCriticalClasses.java fails with jdk.internal.vm.PostVMInitHook not found

[Lois Foltan]

Looks good.
Lois

On 11/27/2018 11:42 AM, Ioi Lam wrote:

https://bugs.openjdk.java.net/browse/JDK-8213275
http://cr.openjdk.java.net/~iklam/jdk12/8213275-ReplaceCriticalClasses-missing-PostVMInitHook.v01/ 



Please review this simple fix. The jdk.internal.vm.PostVMInitHook 
class is not in the openjdk so it should be used in a test in the 
openjdk repo.


It isn't important to test for this particular class. When I wrote the 
test, I was just looking for a few classes that are loaded immediately 
after JVMTI has exited JVMTI_PHASE_PRIMORDIAL, and PostVMInitHook 
happened to be one of those classes.


Since we already test a few other classes such as java/util/Locale, 
the fix is simply to remove PostVMInitHook from the test.


Thanks
- Ioi

Re: RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent

[Lois Foltan]

On 10/23/2018 12:09 AM, Ioi Lam wrote:




On 10/22/18 10:25 AM, Lois Foltan wrote:

On 10/22/2018 1:49 AM, Ioi Lam wrote:


Hi David,

Thanks for the review. Updated webrev:

http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/ 

http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/ 



Hi Ioi,

Looks good.  A couple of comments:

classfile/systemDictionary.cpp
- line#1975 - My preference is to declare the variable sid outside 
the for statement and compare sid == 0 within the for loop conditional.

Do you mean this?

bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
  int sid;
  for (int i = 0; (sid = wk_init_info[i]) != 0; i++) {
    Symbol* symbol = vmSymbols::symbol_at((vmSymbols::SID)sid);
    if (class_name == symbol) {
  return true;
    }
  }
  return false;
}


Yes, I think that's better.

- line#1981 - can you use class_name->fast_compare(symbol) for the 
equality check?



The comments around fast_compare says it's for vtable sorting only.

Right, David pointed that out to me as well.  Comment retracted.




memory/heapShared.cpp
- line#422 could be a ResourceMark rm(THREAD);


Fixed.

Great, thanks!
Lois



Thanks
- Ioi


Thanks,
Lois



More comments below:



On 10/21/18 6:57 PM, David Holmes wrote:

Hi Ioi,

Generally seems okay.

On 22/10/2018 11:15 AM, Ioi Lam wrote:
Re-sending to the correct mailing lists. Please disregard the 
other email.


http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/ 


https://bugs.openjdk.java.net/browse/JDK-8212200

Hi,

CDS has various built-in assumptions that classes loaded by
SystemDictionary::resolve_well_known_classes must not be replaced
by JVMTI ClassFileLoadHook during run time, including

- field offsets computed in JavaClasses::compute_offsets
- the layout of the strings objects in the shared strings table

The "well-known" classes can be replaced by ClassFileLoadHook only
when JvmtiExport::early_class_hook_env() is true. Therefore, the
fix is to disable CDS under this condition.


I'm a little unclear why we have to iterate JvmtiEnv list when this 
has to be checked during JVMTI_PHASE_PRIMORDIAL?


I think you are asking about this new function? I don't like the 
name "early_class_hook_env()". Maybe I should change it to 
"has_early_class_hook_env()"?



bool JvmtiExport::early_class_hook_env() {
  JvmtiEnvIterator it;
  for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
    if (env->early_class_hook_env()) {
  return true;
    }
  }
  return false;
}

This function matches condition in the existing code that would call 
into ClassFileLoadHook:


class JvmtiClassFileLoadHookPoster {
 ...
  void post_all_envs() {
    JvmtiEnvIterator it;
    for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
        ..
    post_to_env(env, true);
    }
  }
...
  void post_to_env(JvmtiEnv* env, bool caching_needed) {
    if (env->phase() == JVMTI_PHASE_PRIMORDIAL && 
!env->early_class_hook_env()) {

  return;
    }


post_all_envs() is called just before a class is about to be loaded 
in the JVM. So if *any* env->early_class_hook_env() returns true, 
there's a chance that it will replace a well-known class.So, as a 
preventive measure, CDS will be disabled.





I have added a few test cases to try to replace shared classes,
including well-known classes and other classes. See
comments in ReplaceCriticalClasses.java for details.

As a clean up, I also renamed all use of "preloaded" in
the source code to "well-known". They refer to the same thing
in HotSpot, so there's no need to use 2 terms. Also, The word
"preloaded" is ambiguous -- it's unclear when "preloading" happens,
and could be confused with what CDS does during archive dump time.


A few specific comments:

src/hotspot/share/classfile/systemDictionary.cpp

+ bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
+   for (int i = 0; ; i++) {
+ int sid = wk_init_info[i];
+ if (sid == 0) {
+   break;
+ }

I take it a zero value is a guaranteed end-of-list sentinel?



Yes. The array is defined just a few lines above:

static const short wk_init_info[] = {
  #define WK_KLASS_INIT_INFO(name, symbol) \
    ((short)vmSymbols::VM_SYMBOL_ENUM_NAME(symbol)),

  WK_KLASSES_DO(WK_KLASS_INIT_INFO)
  #undef WK_KLASS_INIT_INFO
  0
};

Also,

class vmSymbols: AllStatic {
  enum SID {
    NO_SID = 0,
    




+ for (int i=FIRST_WKID; i

Fixed.


---

test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java 



New file should have current copyright year only.


Fixed.

 31  * @comment CDS should not be disabled -- these critical 
classes will be replaced because 
JvmtiExport::early_class_hook_env() is true.


Comment seems contradictory: if we replace critical classes 

Re: RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent

[Lois Foltan]

On 10/22/2018 1:49 AM, Ioi Lam wrote:


Hi David,

Thanks for the review. Updated webrev:

http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/ 

http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/ 



Hi Ioi,

Looks good.  A couple of comments:

classfile/systemDictionary.cpp
- line#1975 - My preference is to declare the variable sid outside the 
for statement and compare sid == 0 within the for loop conditional.
- line#1981 - can you use class_name->fast_compare(symbol) for the 
equality check?


memory/heapShared.cpp
- line#422 could be a ResourceMark rm(THREAD);

Thanks,
Lois



More comments below:



On 10/21/18 6:57 PM, David Holmes wrote:

Hi Ioi,

Generally seems okay.

On 22/10/2018 11:15 AM, Ioi Lam wrote:
Re-sending to the correct mailing lists. Please disregard the other 
email.


http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/ 


https://bugs.openjdk.java.net/browse/JDK-8212200

Hi,

CDS has various built-in assumptions that classes loaded by
SystemDictionary::resolve_well_known_classes must not be replaced
by JVMTI ClassFileLoadHook during run time, including

- field offsets computed in JavaClasses::compute_offsets
- the layout of the strings objects in the shared strings table

The "well-known" classes can be replaced by ClassFileLoadHook only
when JvmtiExport::early_class_hook_env() is true. Therefore, the
fix is to disable CDS under this condition.


I'm a little unclear why we have to iterate JvmtiEnv list when this 
has to be checked during JVMTI_PHASE_PRIMORDIAL?


I think you are asking about this new function? I don't like the name 
"early_class_hook_env()". Maybe I should change it to 
"has_early_class_hook_env()"?



bool JvmtiExport::early_class_hook_env() {
  JvmtiEnvIterator it;
  for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
    if (env->early_class_hook_env()) {
  return true;
    }
  }
  return false;
}

This function matches condition in the existing code that would call 
into ClassFileLoadHook:


class JvmtiClassFileLoadHookPoster {
 ...
  void post_all_envs() {
    JvmtiEnvIterator it;
    for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
        ..
    post_to_env(env, true);
    }
  }
...
  void post_to_env(JvmtiEnv* env, bool caching_needed) {
    if (env->phase() == JVMTI_PHASE_PRIMORDIAL && 
!env->early_class_hook_env()) {

  return;
    }


post_all_envs() is called just before a class is about to be loaded in 
the JVM. So if *any* env->early_class_hook_env() returns true, there's 
a chance that it will replace a well-known class.So, as a preventive 
measure, CDS will be disabled.





I have added a few test cases to try to replace shared classes,
including well-known classes and other classes. See
comments in ReplaceCriticalClasses.java for details.

As a clean up, I also renamed all use of "preloaded" in
the source code to "well-known". They refer to the same thing
in HotSpot, so there's no need to use 2 terms. Also, The word
"preloaded" is ambiguous -- it's unclear when "preloading" happens,
and could be confused with what CDS does during archive dump time.


A few specific comments:

src/hotspot/share/classfile/systemDictionary.cpp

+ bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
+   for (int i = 0; ; i++) {
+ int sid = wk_init_info[i];
+ if (sid == 0) {
+   break;
+ }

I take it a zero value is a guaranteed end-of-list sentinel?



Yes. The array is defined just a few lines above:

static const short wk_init_info[] = {
  #define WK_KLASS_INIT_INFO(name, symbol) \
    ((short)vmSymbols::VM_SYMBOL_ENUM_NAME(symbol)),

  WK_KLASSES_DO(WK_KLASS_INIT_INFO)
  #undef WK_KLASS_INIT_INFO
  0
};

Also,

class vmSymbols: AllStatic {
  enum SID {
    NO_SID = 0,
    




+ for (int i=FIRST_WKID; i

Fixed.


---

test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java 



New file should have current copyright year only.


Fixed.

 31  * @comment CDS should not be disabled -- these critical classes 
will be replaced because JvmtiExport::early_class_hook_env() is true.


Comment seems contradictory: if we replace critical classes then CDS 
should be disabled right??



Fixed.


I expected to see tests that checked for:

"CDS is disabled because early JVMTI ClassFileLoadHook is in use."

in the output. ??



It would have been easy if jtreg lets you check the output of @run 
easily. Instead, your innocent suggestion has turned into 150+ lines 
of new code :-( Maybe "let's write all shell tests in Java" isn't such 
a great idea after all.



Now the test checks that whether CDS is indeed disabled, whether the 
affected class is loaded from the shared archive, etc.


Thanks
- Ioi


Thanks,
David




 > In early e-mails Jiangli wrote:
 >
 > We should consider including more classes from the default classlist
 > in the test. Archived classes loaded du

Re: RFR (trivial) 8210861: Move assert to help diagnose rare RedefineStress crash

[Lois Foltan]

Looks good & trivial.
Lois

On 9/18/2018 12:10 PM, coleen.phillim...@oracle.com wrote:
Summary: assert that Method being marked on stack hasn't been missed 
by previous metadata walk


Ran hs-tier1-7 with no failures, which is unfortunate because this 
assert will tell me where the Method was missed from previous walk.


open webrev at http://cr.openjdk.java.net/~coleenp/8210861.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8210861

Thanks,
Coleen

Re: RFR 8209821: Make JVMTI GetClassLoaderClasses not walk CLDG

[Lois Foltan]

On 8/24/2018 2:59 PM, coleen.phillim...@oracle.com wrote:




On 8/24/18 11:44 AM, Lois Foltan wrote:

On 8/23/2018 8:37 AM, coleen.phillim...@oracle.com wrote:


Summary: And also added function with KlassClosure to remove the hacks.

There are about 10 vmTestbase/nsk/jvmti tests that test various 
parts of this change.  Also ran mach5 tier1-7.


open webrev at http://cr.openjdk.java.net/~coleenp/8209821.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8209821

Thanks,
Coleen


Hi Coleen,

I think this is a good clean up.  Couple of comments.

- memory/universe.cpp
You could make basic_type_classes_do() be a for loop
    for (int i = 0; i < T_VOID+1; i++) {
  closure->do_klass(typeArrayKlassObjs[i]());
  }


Interesting observation.  This is equivalent except T_OBJECT and 
T_ARRAY elements aren't initiatialized.  I believe that the do_klass 
in the closure I am passing will choke on NULL.  I've never understood 
why these statics needed to be duplicated like this, and have tried to 
clean this up before.  Maybe an RFE to do so would be better.

Hi Coleen,

Then just have the for loop be instead  for (int i = T_BOOLEAN; i < 
T_LONG+1; i++).  I'm okay with what you decide.  However, to me the code 
in Universe::metaspace_pointers_do() at line 230-232 looks wrong:


for (int i = 0; i < T_VOID+1; i++) {
    it->push(&_typeArrayKlassObjs[i]);
  }

The VM does not appear to store anything in _typeArrayKlassObjs for 
elements 0-3, so hopefully those elements are NULL.  It should at least 
start that for loop off at T_BOOLEAN.


Thanks,
Lois




- prims/jvmtiGetLoadedClasses.cpp
In JvmtiGetLoadedClasses::getClassLoaderClasses() you could pull the 
call to basic_type_classes_do() from both sections of the if 
statement to line #139


Thanks, I'll fix it.

Thanks for the code review.
Coleen


Thanks,
Lois

Re: RFR 8209821: Make JVMTI GetClassLoaderClasses not walk CLDG

[Lois Foltan]

On 8/23/2018 8:37 AM, coleen.phillim...@oracle.com wrote:


Summary: And also added function with KlassClosure to remove the hacks.

There are about 10 vmTestbase/nsk/jvmti tests that test various parts 
of this change.  Also ran mach5 tier1-7.


open webrev at http://cr.openjdk.java.net/~coleenp/8209821.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8209821

Thanks,
Coleen


Hi Coleen,

I think this is a good clean up.  Couple of comments.

- memory/universe.cpp
You could make basic_type_classes_do() be a for loop
    for (int i = 0; i < T_VOID+1; i++) {
  closure->do_klass(typeArrayKlassObjs[i]());
  }

- prims/jvmtiGetLoadedClasses.cpp
In JvmtiGetLoadedClasses::getClassLoaderClasses() you could pull the 
call to basic_type_classes_do() from both sections of the if statement 
to line #139


Thanks,
Lois

Re: RFR: (S): JDK-8204308 : SA: serviceability/sa/TestInstanceKlassSize*.java fails when running in CDS mode

[Lois Foltan]

Hi Jini,

This looks good.  I have the same questions as Coleen about the tests on 
the ProblemList.


Thanks,
Lois

On 8/24/2018 4:04 AM, Jini George wrote:

Hello!

Requesting reviews for:
https://bugs.openjdk.java.net/browse/JDK-8204308

Webrev: http://cr.openjdk.java.net/~jgeorge/8204308/webrev.00/index.html

The proposed fix is to use longs instead of ints while computing the 
identity hash of klass symbols. It mimics the implementation of 
Symbol::identity_hash() in "src/hotspot/share/oops/symbol.hpp".


Thanks!
Jini.

Re: RFR 8149790: NegativeArraySizeException with hprof

[Lois Foltan]

George,

I think this looks good.  Copyright needs updating in 
FileReadBuffer.java, MappedReadBuffer.java & ReadBuffer.java.


Thanks,
Lois

On 8/14/2017 11:31 AM, George Triantafillou wrote:
Please review this change to fix NegativeArraySizeException test 
failures with hprof:


JBS: https://bugs.openjdk.java.net/browse/JDK-8149790
webrev: 
http://cr.openjdk.java.net/~gtriantafill/8149790-webrev/webrev/index.html 
 



The original patch was contributed by Andreas Eriksson.  Tested 
locally on Linux-x64 and with RBT tiers 2 through 5.


Thanks.

-George

Re: RFR (XXS): 8171226 simple typo in the JVMTI spec

[Lois Foltan]

Looks good.
Lois

On 12/14/2016 5:30 AM, serguei.spit...@oracle.com wrote:

Need one reviewer for a trivial typo fix in the JVMTI spec:
  https://bugs.openjdk.java.net/browse/JDK-8171226


The suggested fix is ("bot" => "not"):

--- a/src/share/vm/prims/jvmti.xmlThu Dec 08 15:49:29 2016 +0100
+++ b/src/share/vm/prims/jvmti.xmlWed Dec 14 02:24:51 2016 -0800
@@ -3370,7 +3370,7 @@
   
   
 Reference from a class to its superclass.
-A callback is bot sent if the superclass is 
java.lang.Object.
+A callback is not sent if the superclass is 
java.lang.Object.

 Note: loaded classes define superclasses via a constant pool
 reference, so the referenced superclass may also be 
reported with
 a JVMTI_HEAP_REFERENCE_CONSTANT_POOL 
reference kind.



Thanks,
Serguei

Re: RFR(S): 8170672: Event-based tracing to support classloader instances

[Lois Foltan]

On 12/5/2016 6:33 PM, Markus Gronlund wrote:

Greetings,

  


Kindly asking for reviews for the following changeset:

  


Bug/Enh: https://bugs.openjdk.java.net/browse/JDK-8170672

Webrev: http://cr.openjdk.java.net/~mgronlun/8170672/webrev01/

(this work is covered by an FC exception)


Hi Markus,

Looks good.  One comment:

src/share/vm/trace/traceStream.hpp
- line #121 - you might want to check not only if the 
class_loader_name_oop is NULL but also for the empty string?  Much like 
the code does in runtime/SharedRuntime.cpp class_loader_and_module_name()?


Thanks,
Lois



  


Summary:

Event-based tracing previously had little information about class loaders; a 
class loader was essentially treated no different than from a regular class 
(type information only).

With JDK9, supported has been added to java.lang.ClassLoader to associate a 
name with an individual class loader instance.

  


This changeset will allow the name information of individual class loader 
instances to be provided by the event-based tracing framework.

  


Aux info:

Some folding of the numerous macros completed as well.

  


Thanks in advance
Markus

  

Re: RFR: 8165827: Support private interface methods in JNI, JDWP, JDI and JDB

[Lois Foltan]

Looks good David!
Lois

On 10/17/2016 11:59 PM, David Holmes wrote:

Hi Lois, Dan, Serguei,

Went to push this today and realized I had left off the updated JNI 
method lookup tests. As I said in the bug report JNI behaves as 
expected, but there weren't any testcases so I added them:


http://cr.openjdk.java.net/~dholmes/8165827/webrev.hotspot/

Thanks,
David

On 11/10/2016 11:55 AM, David Holmes wrote:

Turns out the only place changes were needed were in JDI.

Bug: https://bugs.openjdk.java.net/browse/JDK-8165827

webrev: http://cr.openjdk.java.net/~dholmes/8165827/webrev/

The spec change in ObjectReference is very simple and there is a CCC
request in progress to ratify that change.

The implementation change in ObjectReferenceImpl mirrors the updated
spec and use the same format as already present in the class version of
the check method.

The test is a little more complex. This is obviously an extension to
what is already tested in InterfaceMethodsTest. However IMT has a number
of problem with the way it is currently written [1] - specifically it
doesn't properly separate method lookup from method invocation. So I've
added the capability to separate lookup and invocation for use with the
private interface methods - I have not tried to address shortcomings of
the existing tests. Though I did fix the return value checking logic!
And did some clarifying comments and renaming in a couple of place.

Still on the test I can't add the negative tests I would like to add
because they actually pass due to a different long standing bug in JDI -
[2]. So the actual private interface method testing is very simple: can
I get the Method from the InterfaceType for the interface declaring the
method? Can I then invoke that method on an instance of a class that
implements the interface.

Thanks,
David

[1] https://bugs.openjdk.java.net/browse/JDK-8166453
[2] https://bugs.openjdk.java.net/browse/JDK-8167416

Re: RFR: 8165827: Support private interface methods in JNI, JDWP, JDI and JDB

[Lois Foltan]

Hi David,
This looks good and I like the improvements you made to the test.
Lois

On 10/10/2016 9:55 PM, David Holmes wrote:

Turns out the only place changes were needed were in JDI.

Bug: https://bugs.openjdk.java.net/browse/JDK-8165827

webrev: http://cr.openjdk.java.net/~dholmes/8165827/webrev/

The spec change in ObjectReference is very simple and there is a CCC 
request in progress to ratify that change.


The implementation change in ObjectReferenceImpl mirrors the updated 
spec and use the same format as already present in the class version 
of the check method.


The test is a little more complex. This is obviously an extension to 
what is already tested in InterfaceMethodsTest. However IMT has a 
number of problem with the way it is currently written [1] - 
specifically it doesn't properly separate method lookup from method 
invocation. So I've added the capability to separate lookup and 
invocation for use with the private interface methods - I have not 
tried to address shortcomings of the existing tests. Though I did fix 
the return value checking logic! And did some clarifying comments and 
renaming in a couple of place.


Still on the test I can't add the negative tests I would like to add 
because they actually pass due to a different long standing bug in JDI 
- [2]. So the actual private interface method testing is very simple: 
can I get the Method from the InterfaceType for the interface 
declaring the method? Can I then invoke that method on an instance of 
a class that implements the interface.


Thanks,
David

[1] https://bugs.openjdk.java.net/browse/JDK-8166453
[2] https://bugs.openjdk.java.net/browse/JDK-8167416

Re: RFR 8081219: hs_err improvement: Add event logging for class redefinition to the hs_err file

[Lois Foltan]

Looks good!
Lois

On 6/2/2015 3:58 PM, Coleen Phillimore wrote:
Summary: Use the Events::log function to save redefined classes for 
output to the hs_err file.


This change adds lines to the hs_err_pid file which names classes 
redefined:


Classes redefined (2 events):
Event: 11.867 Thread 0x7fd378199800 redefined class 
name=RedefineRunningMethods$B, count=1
Event: 26.746 Thread 0x7fd378199800 redefined class 
name=RedefineRunningMethods$B, count=2


Tested with intentional crash to see hs_err_pid files, also tested 
without intentional crashes vm.redefine.testlist.


open webrev at http://cr.openjdk.java.net/~coleenp/8081219/
bug link https://bugs.openjdk.java.net/browse/JDK-8081219

Thanks,
Coleen

Re: RFR: JDK-8075586: add @modules as needed to the open hotspot tests

[Lois Foltan]

On 3/25/2015 10:38 AM, Alexander Kulyakhtin wrote:

Hi

Please, find the updated review for the bulk @modules change at the link below.

We have fixed the copyrights and the files mentioned in the mail from Lois.

http://cr.openjdk.java.net/~eistepan/~akulyakhtin/8075586/index.html


Thank you very much for adding the copyrights in this edit.  I have 
reviewed and it all looks good.  Just a minor comment:


   test/gc/arguments/TestUseCompressedOopsErgo.java
   test/serviceability/jvmti/GetObjectSizeOverflow.java

Both of these tests need a comma between "2015 Oracle".  I don't need to 
see another webrev for this.


Thanks,
Lois




Best regards,
Alex



- Original Message -
From: lois.fol...@oracle.com
To: yekaterina.kantser...@oracle.com
Cc: serviceability-dev@openjdk.java.net, staffan.lar...@oracle.com, 
hotspot-...@openjdk.java.net, alexander.kulyakh...@oracle.com, 
alexandre.il...@oracle.com
Sent: Tuesday, March 24, 2015 3:57:54 PM GMT +04:00 Abu Dhabi / Muscat
Subject: Re: RFR: JDK-8075586: add @modules as needed to the open hotspot tests


This looks good, thank you for making these changes!  A couple of
comments that I don't feel need another webrev but should be fixed
before pushing.

  - copyrights on all the tests need to be updated
  - the following tests have a blank comment line before the new
"@modules" line that could be removed
test/gc/metaspace/TestMetaspacePerfCounters.java
test/runtime/contended/Basic.java
test/compiler/jsr292/CreatesInterfaceDotEqualsCallInfo.java
test/compiler/cpuflags/RestoreMXCSR.java
test/compiler/debug/VerifyAdapterSharing.java

Thanks,
Lois

On 3/24/2015 8:09 AM, Yekaterina Kantserova wrote:

Notifying hotspot-dev as well.

// Katja



On 03/24/2015 11:48 AM, Alexander Kulyakhtin wrote:

Could the reviewers, please, have a look at the proposed changes below?

In addition, we are going to make a change to the TEST.ROOT file as
indicated by Staffan in the mail below.

Do you think the changes (plus the one-line change to the TEST.ROOT)
can be pushed into the jdk?

Best regards,
Alex

- Original Message -
From: staffan.lar...@oracle.com
To: alexander.kulyakh...@oracle.com
Cc: serviceability-dev@openjdk.java.net, alexandre.il...@oracle.com
Sent: Friday, March 20, 2015 7:39:10 PM GMT +04:00 Abu Dhabi / Muscat
Subject: Re: RFR: JDK-8075586: add @modules as needed to the open
hotspot tests

I haven’t looked at the changes in detail, but please change the
requiredVersion in TEST.ROOT to 4.1 b11 as part of this change.

Thanks,
/Staffan


On 20 mar 2015, at 13:16, Alexander Kulyakhtin
 wrote:

Hi,

Could you, please, review the fix below.

CR: https://bugs.openjdk.java.net/browse/JDK-8075586
webrev:
http://cr.openjdk.java.net/~tpivovarova/akulyakh/8075586/webrev.00/

The fix adds @modules keyword to the existing hotspot tests, as
needed, so that the tests can access the required API when the new
modular architecture is in place.

Best regards,
Alex

Re: RFR: JDK-8075586: add @modules as needed to the open hotspot tests

[Lois Foltan]

This looks good, thank you for making these changes!  A couple of 
comments that I don't feel need another webrev but should be fixed 
before pushing.


- copyrights on all the tests need to be updated
- the following tests have a blank comment line before the new 
"@modules" line that could be removed

  test/gc/metaspace/TestMetaspacePerfCounters.java
  test/runtime/contended/Basic.java
  test/compiler/jsr292/CreatesInterfaceDotEqualsCallInfo.java
  test/compiler/cpuflags/RestoreMXCSR.java
  test/compiler/debug/VerifyAdapterSharing.java

Thanks,
Lois

On 3/24/2015 8:09 AM, Yekaterina Kantserova wrote:

Notifying hotspot-dev as well.

// Katja



On 03/24/2015 11:48 AM, Alexander Kulyakhtin wrote:

Could the reviewers, please, have a look at the proposed changes below?

In addition, we are going to make a change to the TEST.ROOT file as 
indicated by Staffan in the mail below.


Do you think the changes (plus the one-line change to the TEST.ROOT) 
can be pushed into the jdk?


Best regards,
Alex

- Original Message -
From: staffan.lar...@oracle.com
To: alexander.kulyakh...@oracle.com
Cc: serviceability-dev@openjdk.java.net, alexandre.il...@oracle.com
Sent: Friday, March 20, 2015 7:39:10 PM GMT +04:00 Abu Dhabi / Muscat
Subject: Re: RFR: JDK-8075586: add @modules as needed to the open 
hotspot tests


I haven’t looked at the changes in detail, but please change the 
requiredVersion in TEST.ROOT to 4.1 b11 as part of this change.


Thanks,
/Staffan

On 20 mar 2015, at 13:16, Alexander Kulyakhtin 
 wrote:


Hi,

Could you, please, review the fix below.

CR: https://bugs.openjdk.java.net/browse/JDK-8075586
webrev: 
http://cr.openjdk.java.net/~tpivovarova/akulyakh/8075586/webrev.00/


The fix adds @modules keyword to the existing hotspot tests, as 
needed, so that the tests can access the required API when the new 
modular architecture is in place.


Best regards,
Alex

Re: RFR: 7164841: Improvements to the GC log file rotation

[Lois Foltan]

Hi Yumin,

I do have some follow on review comments:

arguments.cpp - looks good, no comments

ostream.hpp - looks good, no comments

ostream.cpp -

- Have you tried a file name that is 255 characters?  It would seem 
that after you appended the pid + timestamp + .current + # you could 
overrun this buffer?


   439 #define FILENAMEBUFLEN 256
and subsequent use at
   466 char tempbuf[FILENAMEBUFLEN];
467 jio_snprintf(tempbuf, sizeof(tempbuf), "%s.%d" CURRENTAPPX,
   _file_name, _cur_file_num);

- I would also like to point out in line #467, there may not be a 
need for "sizeof(tempbuf)", isn't it just FILENAMEBUFLEN?
  Please check the use of "sizeof()" in the jio_sprintf statements, 
I think all are known.


   - Related to my first comment.  If you have a time_msg that is 
FILENAMEBUFLEN and you try to concatenate it with a file_name that is 
FILENAMEBUFLEN + the

 os::local_time_string, you've overrun your buffer.

   493 char time_msg[FILENAMEBUFLEN];
 494 char time_str[EXTRACHARLEN];
 495 char current_file_name[FILENAMEBUFLEN];
   496 char renamed_file_name[FILENAMEBUFLEN];
 ...
 530 jio_snprintf(time_msg, sizeof(time_msg), "%s GC log file
   has reached the"
 531 " maximum size.
   Saved as %s\n",
 532 os::local_time_string((char *)time_str, sizeof(time_str)),
 533 renamed_file_name);

- Line #538 dealing with the rename of .current.#.  I 
don't prefer the use of .current. Take for example a user
  specified on the command line -XX:NumberOfGCLogFiles=5, but there 
is enough -Xloggc info generated to fit in 7 files.  This
  situation will cause the log file rotation to rotate back on 
itself.  So, file #0 will be reopened and used as the 6th file, and
  then the rotation will progress and finish dumping Xloggc 
information into the last file which would be .current.1,

  correct?  So a user would be left with the following files.

file_name.0 (which now has a later timestamp in 
its name than file # 1 thru 4)

file_name.1
file_name.2
file_name.3
file_name.4
file_name.current.1

  I find this confusing, would you consider having the -Xloggc 
information be dumped into the current #'ed file directly?


- Line 587 FLAG_SET_DEFAULT(UseGCLogFileRotation, false);
  I like that you implemented the idea to turn off GC log file 
rotation and continue with the current file if you can't open the next 
file, thank you.


Thank you,
Lois

On 8/27/2013 11:32 PM, Yumin Qi wrote:

Hi,

 Based on the discussion, I updated the webrev, and in this version
1) deleted unused rotatingFileStream  constructor, which keep code 
shorter.

2) removed reformat_filename in previous version.
3) still keep original design, that if no rotation, just use file name 
given by -Xloggc:.


Others are basically same.

Please take a look and comment.

http://cr.openjdk.java.net/~minqi/7164841/webrev02 



Thanks
Yumin

On 8/27/2013 10:13 AM, Tao Mao wrote:



On 8/27/13 1:01 AM, Bengt Rutisson wrote:


Yumin,

On 8/26/13 7:01 PM, Yumin Qi wrote:

Bengt,

  Thanks for your comments.
  Yes, as you said, the purpose of rotating logs is primarily 
targeted for saving disk space. This bug is supplying customers 
another option to prevent the previous gc logs from removed by 
restart app without making copy. Now with this pid and time stamp 
included in file name,  we have more options for users. It is up to 
user to make decision to or not to keep the logs. We cannot handle 
all the requests in JVM, but we can offer the choices for users I 
think. Any way, with either the previous rotating version, or the 
one I am working, the logs will not grow constantly without limit 
to blow storage out as long as users give enough attention.


  My concern is for no log rotation, should we still use time stamp 
in file name? I have one version for this, I hope get more comments 
for that.


Sorry if I wasn't clear before. I am not worried about the case when 
log rotation is turned on. What I was worried about was the case 
where a user is not using log rotation but is still piping the GC 
output to a file. That file will be overwritten every time the VM is 
restarted. If we add time stamps to the file name for this case then 
the file will not be overwritten. I think that is a bit of a scary 
change. That's all.


If you are worried about the case where a user is not using log 
rotation but creating a new file for each restart, you should be 
almost equivalently worried about the case where a user is using log 
rotation and having many rotated logs created which are different for 
each VM restart. Basically, we are creating even more files over 
time, which falls into your concern.


At this point, I'm