Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

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

Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

Mandy Chung
A tiny startup fix - useArrayDeque instead of LinkedList for
ClassLoader.NativeLibrary which is typically loaded at startup for
example when loading a JAR file.

Thanks
Mandy

diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java
b/src/java.base/share/classes/java/lang/ClassLoader.java
--- a/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -38,6 +38,7 @@
  import java.security.PrivilegedAction;
  import java.security.ProtectionDomain;
  import java.security.cert.Certificate;
+import java.util.ArrayDeque;
  import java.util.Arrays;
  import java.util.Collections;
  import java.util.Deque;
@@ -45,7 +46,6 @@
  import java.util.HashMap;
  import java.util.HashSet;
  import java.util.Hashtable;
-import java.util.LinkedList;
  import java.util.Map;
  import java.util.NoSuchElementException;
  import java.util.Objects;
@@ -2496,7 +2496,7 @@
          }

          // native libraries being loaded
-        static Deque<NativeLibrary> nativeLibraryContext = new
LinkedList<>();
+        static Deque<NativeLibrary> nativeLibraryContext = new
ArrayDeque<>();

          /*
           * The run() method will be invoked when this class loader
becomes
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

Martin Buchholz-3
Google decided that LinkedList is almost never the best choice, so any use
of LinkedList is discouraged.

ArrayDeque's backing array never shrinks, so you might want to give it an
explicit initial size.

On Wed, Dec 6, 2017 at 4:33 PM, mandy chung <[hidden email]> wrote:

> A tiny startup fix - useArrayDeque instead of LinkedList for
> ClassLoader.NativeLibrary which is typically loaded at startup for example
> when loading a JAR file.
>
> Thanks
> Mandy
>
> diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java
> b/src/java.base/share/classes/java/lang/ClassLoader.java
> --- a/src/java.base/share/classes/java/lang/ClassLoader.java
> +++ b/src/java.base/share/classes/java/lang/ClassLoader.java
> @@ -38,6 +38,7 @@
>  import java.security.PrivilegedAction;
>  import java.security.ProtectionDomain;
>  import java.security.cert.Certificate;
> +import java.util.ArrayDeque;
>  import java.util.Arrays;
>  import java.util.Collections;
>  import java.util.Deque;
> @@ -45,7 +46,6 @@
>  import java.util.HashMap;
>  import java.util.HashSet;
>  import java.util.Hashtable;
> -import java.util.LinkedList;
>  import java.util.Map;
>  import java.util.NoSuchElementException;
>  import java.util.Objects;
> @@ -2496,7 +2496,7 @@
>          }
>
>          // native libraries being loaded
> -        static Deque<NativeLibrary> nativeLibraryContext = new
> LinkedList<>();
> +        static Deque<NativeLibrary> nativeLibraryContext = new
> ArrayDeque<>();
>
>          /*
>           * The run() method will be invoked when this class loader becomes
>
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

Mandy Chung


On 12/6/17 6:08 PM, Martin Buchholz wrote:
> Google decided that LinkedList is almost never the best choice, so any
> use of LinkedList is discouraged.
>
> ArrayDeque's backing array never shrinks, so you might want to give it
> an explicit initial size.
>

The initial size is 16 which is okay.  I can make it smaller instead:

-        static Deque<NativeLibrary> nativeLibraryContext = new
LinkedList<>();
+        static Deque<NativeLibrary> nativeLibraryContext = new
ArrayDeque<>(8);

Mandy

> On Wed, Dec 6, 2017 at 4:33 PM, mandy chung <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     A tiny startup fix - useArrayDeque instead of LinkedList for
>     ClassLoader.NativeLibrary which is typically loaded at startup for
>     example when loading a JAR file.
>
>     Thanks
>     Mandy
>
>     diff --git
>     a/src/java.base/share/classes/java/lang/ClassLoader.java
>     b/src/java.base/share/classes/java/lang/ClassLoader.java
>     --- a/src/java.base/share/classes/java/lang/ClassLoader.java
>     +++ b/src/java.base/share/classes/java/lang/ClassLoader.java
>     @@ -38,6 +38,7 @@
>      import java.security.PrivilegedAction;
>      import java.security.ProtectionDomain;
>      import java.security.cert.Certificate;
>     +import java.util.ArrayDeque;
>      import java.util.Arrays;
>      import java.util.Collections;
>      import java.util.Deque;
>     @@ -45,7 +46,6 @@
>      import java.util.HashMap;
>      import java.util.HashSet;
>      import java.util.Hashtable;
>     -import java.util.LinkedList;
>      import java.util.Map;
>      import java.util.NoSuchElementException;
>      import java.util.Objects;
>     @@ -2496,7 +2496,7 @@
>              }
>
>              // native libraries being loaded
>     -        static Deque<NativeLibrary> nativeLibraryContext = new
>     LinkedList<>();
>     +        static Deque<NativeLibrary> nativeLibraryContext = new
>     ArrayDeque<>();
>
>              /*
>               * The run() method will be invoked when this class
>     loader becomes
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

Alan Bateman


On 07/12/2017 05:04, mandy chung wrote:

>
>
> On 12/6/17 6:08 PM, Martin Buchholz wrote:
>> Google decided that LinkedList is almost never the best choice, so
>> any use of LinkedList is discouraged.
>>
>> ArrayDeque's backing array never shrinks, so you might want to give
>> it an explicit initial size.
>>
>
> The initial size is 16 which is okay.  I can make it smaller instead:
>
> -        static Deque<NativeLibrary> nativeLibraryContext = new
> LinkedList<>();
> +        static Deque<NativeLibrary> nativeLibraryContext = new
> ArrayDeque<>(8);
This looks okay. It seems unlikely that hundreds of native libraries
will be loaded, then unloaded, and the backing array being a memory concern.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

Claes Redestad


On 2017-12-07 14:39, Alan Bateman wrote:

>
>
> On 07/12/2017 05:04, mandy chung wrote:
>>
>>
>> On 12/6/17 6:08 PM, Martin Buchholz wrote:
>>> Google decided that LinkedList is almost never the best choice, so
>>> any use of LinkedList is discouraged.
>>>
>>> ArrayDeque's backing array never shrinks, so you might want to give
>>> it an explicit initial size.
>>>
>>
>> The initial size is 16 which is okay.  I can make it smaller instead:
>>
>> -        static Deque<NativeLibrary> nativeLibraryContext = new
>> LinkedList<>();
>> +        static Deque<NativeLibrary> nativeLibraryContext = new
>> ArrayDeque<>(8);

+1

> This looks okay. It seems unlikely that hundreds of native libraries
> will be loaded, then unloaded, and the backing array being a memory
> concern.

It also needs to load said hundreds of native libraries in parallel for
this context queue to grow in the first place, right?

So I'm not concerned, but on that tangent I have been wondering for some
time why ArrayDeque doesn't have a
trimToSize method like ArrayList does. Theoretically it'd be nice to
have some means to allow periodically taking a
look at persistent queues like this one and trim the backing arrays down
if they ever outgrow their purpose. I guess
it's just never become a real problem...

/Claes

Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

Mandy Chung
In reply to this post by Alan Bateman


On 12/7/17 5:39 AM, Alan Bateman wrote:

>
>
> On 07/12/2017 05:04, mandy chung wrote:
>>
>>
>> On 12/6/17 6:08 PM, Martin Buchholz wrote:
>>> Google decided that LinkedList is almost never the best choice, so
>>> any use of LinkedList is discouraged.
>>>
>>> ArrayDeque's backing array never shrinks, so you might want to give
>>> it an explicit initial size.
>>>
>>
>> The initial size is 16 which is okay.  I can make it smaller instead:
>>
>> -        static Deque<NativeLibrary> nativeLibraryContext = new
>> LinkedList<>();
>> +        static Deque<NativeLibrary> nativeLibraryContext = new
>> ArrayDeque<>(8);
> This looks okay. It seems unlikely that hundreds of native libraries
> will be loaded, then unloaded, and the backing array being a memory
> concern.

Yup, it's not a big concern.   I believe the number of elements in this
deque is typically very small number.   This deque is used to keep the
native libraries are being loaded and its context used by JNI FindClass
when called from JNI_OnLoad.

Mandy

Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8193159: Reduce the number of classes loaded due to NativeLibrary

Martin Buchholz-3
+1