RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

Claes Redestad
Hi,

various resource encapsulation changes in 9+148 meant an uptick in
footprint and startup times for certain applications.

While some of this regression is hard to avoid[1] (opening readers,
touching more memory mapped pages etc), a large part is due to simple
java allocation churn, some of which can be optimized away by reducing
the number of objects we create when scanning modules for resources.

Bug: https://bugs.openjdk.java.net/browse/JDK-8175561
Webrev: http://cr.openjdk.java.net/~redestad/8175561/jdk.01/

Thanks!

/Claes

[1] Things we could do in the future to improve further include adding
an index to the jimage (too late for 9) or marking modules with no
non-encapsulated resources as such so that they can be skipped.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

Jim Laskey (Oracle)
+1

Sent from my iPhone

> On Feb 23, 2017, at 6:57 PM, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> various resource encapsulation changes in 9+148 meant an uptick in
> footprint and startup times for certain applications.
>
> While some of this regression is hard to avoid[1] (opening readers,
> touching more memory mapped pages etc), a large part is due to simple
> java allocation churn, some of which can be optimized away by reducing
> the number of objects we create when scanning modules for resources.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175561
> Webrev: http://cr.openjdk.java.net/~redestad/8175561/jdk.01/
>
> Thanks!
>
> /Claes
>
> [1] Things we could do in the future to improve further include adding
> an index to the jimage (too late for 9) or marking modules with no non-encapsulated resources as such so that they can be skipped.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

Mandy Chung
In reply to this post by Claes Redestad

> On Feb 23, 2017, at 2:57 PM, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> various resource encapsulation changes in 9+148 meant an uptick in
> footprint and startup times for certain applications.
>
> While some of this regression is hard to avoid[1] (opening readers,
> touching more memory mapped pages etc), a large part is due to simple
> java allocation churn, some of which can be optimized away by reducing
> the number of objects we create when scanning modules for resources.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175561
> Webrev: http://cr.openjdk.java.net/~redestad/8175561/jdk.01/
>


ImageStringsReader.java
   Should the hashCode methods taking byte[] parameter be removed?

ImageLocation.java
   I suggest to add the comment to describe the expect format comparing to the name. something like  “/<module>/[<parent>/]<base>[.<ext>]” and a comment for each if-statement what segment is checking.

I have carefully reviewed the change which attempts to inline the code and write to avoid object allocation.  It looks correct to me.  

Since jimage is a sensitive area, I suggest to run PIT and do the automated hotspot testing.

Mandy


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

Claes Redestad
In reply to this post by Claes Redestad
Hi,

thanks Mandy and Jim for reviewing!

However, I've found enough evidence now that we should this
one step further and eliminating the allocation in
BasicImageReader::findLocation(String, String), which completely
gets rid of the regressions we're seeing:

http://cr.openjdk.java.net/~redestad/8175561/jdk.02/
http://cr.openjdk.java.net/~redestad/8175561/jdk.01.02.diff/

Working through the details on how to do this it became clear
that stringing together ImageStringsReader.hashCode - as done in
ImageLocationWriter - is likely a bug in the making since it's not
guaranteed to give equivalent results to applying the same method
to the whole string. I took the liberty to fix this, and ran the final
patch through several JDK and HotSpot test lists without any surprises.

Thanks!

/Claes

On 02/23/2017 10:57 PM, Claes Redestad wrote:

> Hi,
>
> various resource encapsulation changes in 9+148 meant an uptick in
> footprint and startup times for certain applications.
>
> While some of this regression is hard to avoid[1] (opening readers,
> touching more memory mapped pages etc), a large part is due to simple
> java allocation churn, some of which can be optimized away by reducing
> the number of objects we create when scanning modules for resources.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175561
> Webrev: http://cr.openjdk.java.net/~redestad/8175561/jdk.01/
>
> Thanks!
>
> /Claes
>
> [1] Things we could do in the future to improve further include adding
> an index to the jimage (too late for 9) or marking modules with no
> non-encapsulated resources as such so that they can be skipped.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

Mandy Chung

> On Feb 27, 2017, at 6:55 PM, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> thanks Mandy and Jim for reviewing!
>
> However, I've found enough evidence now that we should this
> one step further and eliminating the allocation in
> BasicImageReader::findLocation(String, String), which completely
> gets rid of the regressions we're seeing:
>
> http://cr.openjdk.java.net/~redestad/8175561/jdk.02/
> http://cr.openjdk.java.net/~redestad/8175561/jdk.01.02.diff/
>

This looks correct but we need Jim to confirm the bug you spotted ImageLocationWriter.

  59     public static int hashCode(String string) {

and all of other hashCode methods.

Nit: "String name" should work as it matches the parameter in the caller method.  Or `s` might be better than “String string”.

 153     static boolean verify(String module, String name,
 154                 final long[] attributes, final ImageStrings strings) {
 67     private static boolean verifyName(String name, int index, final int length,
 168             final long[] attributes, final ImageStrings strings) {

Nit: some final and some non-final parameters and better to be consistent. Any reason why you mark it “final”?

No need for a new webrev.

Thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

Jim Laskey (Oracle)
My only concern at this point is if Claes and I get hit by a bus, no one will be able to figure this out.  I recommend that next release that we switch to character based hash.  This means some minor complexity in the C code but I think the java code will be much simpler (and the C not so bad.)


> On Mar 1, 2017, at 7:42 PM, Mandy Chung <[hidden email]> wrote:
>
>
>> On Feb 27, 2017, at 6:55 PM, Claes Redestad <[hidden email]> wrote:
>>
>> Hi,
>>
>> thanks Mandy and Jim for reviewing!
>>
>> However, I've found enough evidence now that we should this
>> one step further and eliminating the allocation in
>> BasicImageReader::findLocation(String, String), which completely
>> gets rid of the regressions we're seeing:
>>
>> http://cr.openjdk.java.net/~redestad/8175561/jdk.02/
>> http://cr.openjdk.java.net/~redestad/8175561/jdk.01.02.diff/
>>
>
> This looks correct but we need Jim to confirm the bug you spotted ImageLocationWriter.
>
>  59     public static int hashCode(String string) {
>
> and all of other hashCode methods.
>
> Nit: "String name" should work as it matches the parameter in the caller method.  Or `s` might be better than “String string”.
>
> 153     static boolean verify(String module, String name,
> 154                 final long[] attributes, final ImageStrings strings) {
> 67     private static boolean verifyName(String name, int index, final int length,
> 168             final long[] attributes, final ImageStrings strings) {
>
> Nit: some final and some non-final parameters and better to be consistent. Any reason why you mark it “final”?
>
> No need for a new webrev.
>
> Thanks
> Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175561: Memory churn in jimage code affects startup after resource encapsulation changes

Claes Redestad
Thanks for reviews, Jim and Mandy! I've pushed the change...

On 03/02/2017 12:06 AM, Jim Laskey (Oracle) wrote:
> My only concern at this point is if Claes and I get hit by a bus, no one will be able to figure this out.  I recommend that next release that we switch to character based hash.  This means some minor complexity in the C code but I think the java code will be much simpler (and the C not so bad.)

I agree that it'd be interesting to explore this in the future.

/Claes

>
>
>> On Mar 1, 2017, at 7:42 PM, Mandy Chung <[hidden email]> wrote:
>>
>>
>>> On Feb 27, 2017, at 6:55 PM, Claes Redestad <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> thanks Mandy and Jim for reviewing!
>>>
>>> However, I've found enough evidence now that we should this
>>> one step further and eliminating the allocation in
>>> BasicImageReader::findLocation(String, String), which completely
>>> gets rid of the regressions we're seeing:
>>>
>>> http://cr.openjdk.java.net/~redestad/8175561/jdk.02/
>>> http://cr.openjdk.java.net/~redestad/8175561/jdk.01.02.diff/
>>>
>> This looks correct but we need Jim to confirm the bug you spotted ImageLocationWriter.
>>
>>   59     public static int hashCode(String string) {
>>
>> and all of other hashCode methods.
>>
>> Nit: "String name" should work as it matches the parameter in the caller method.  Or `s` might be better than “String string”.
>>
>> 153     static boolean verify(String module, String name,
>> 154                 final long[] attributes, final ImageStrings strings) {
>> 67     private static boolean verifyName(String name, int index, final int length,
>> 168             final long[] attributes, final ImageStrings strings) {
>>
>> Nit: some final and some non-final parameters and better to be consistent. Any reason why you mark it “final”?
>>
>> No need for a new webrev.
>>
>> Thanks
>> Mandy