Re: [cp-patches] [RFC PATCH 2/4] Optimize emptySet/Map/List() in Collections class

[Ivan Maidanski]

Hi Andrew,

You're right about emptyList() invocation from subList.
Thanks.

Updated (and rebased) commit: 
https://github.com/ivmai/classpath/commit/ef3af4f38970f7cea3d5b5a7d98a617f7b23da4e
(branch: https://github.com/ivmai/classpath/tree/ivmai4review-v3)

diff --git a/ChangeLog b/ChangeLog
index f78f393..5805463 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2012-10-16  Ivan Maidanski  iv...@mail.ru
+
+    * java/util/Collections.java:
+    (emptySet(), EmptySet.iterator(), emptyList(), emptyMap(),
+    EmptyMap.entrySet(), EmptyMap.keySet(), EmptyMap.values()): Suppress
+    unchecked warnings.
+    (emptySet(), emptyList(), emptyMap()): Don't create new instance (use
+    the corresponding immutable container instance); remove FIXME.
+    (EmptySet.equals(Object), EmptyList.equals(Object),
+    EmptyMap.entrySet(), EmptyMap.equals(Object), EmptyMap.keySet(),
+    EmptyMap.values()): Add generic typing.
+    (SingletonList.subList(int, int)): Use emptyList() instead of
+    EMPTY_LIST (to eliminate unchecked warning).
+    (SynchronizedCollection.toArray(T[])): Rename T type to E (to
+    suppress compiler warning about type hiding).
+
 2012-10-15  Andrew John Hughes  gnu_and...@member.fsf.org
 
     * configure.ac: Set to 0.99.1pre, as
diff --git a/java/util/Collections.java b/java/util/Collections.java
index 828c6ec..b970dd8 100644
--- a/java/util/Collections.java
+++ b/java/util/Collections.java
@@ -120,10 +120,10 @@ public class Collections
    * @return an empty parameterized set.
    * @since 1.5
    */
+  @SuppressWarnings(unchecked)
   public static final T SetT emptySet()
   {
-    /* FIXME: Could this be optimized? */
-    return new EmptySetT();
+    return (SetT) EMPTY_SET;
   }
 
   /**
@@ -161,6 +161,7 @@ public class Collections
  * @return A non-iterating iterator.
  */
 // This is really cheating! I think it's perfectly valid, though.
+    @SuppressWarnings(unchecked)
 public IteratorT iterator()
 {
   return (IteratorT) EMPTY_LIST.iterator();
@@ -196,7 +197,7 @@ public class Collections
  */
 public boolean equals(Object o)
 {
-  return o instanceof Set  ((Set) o).isEmpty();
+  return o instanceof Set?  ((Set?) o).isEmpty();
 }
 
 /**
@@ -288,10 +289,10 @@ public class Collections
    * @return an empty parameterized list.
    * @since 1.5
    */
+  @SuppressWarnings(unchecked)
   public static final T ListT emptyList()
   {
-    /* FIXME: Could this be optimized? */
-    return new EmptyListT();
+    return (ListT) EMPTY_LIST;
   }
 
   /**
@@ -369,7 +370,7 @@ public class Collections
  */
 public boolean equals(Object o)
 {
-  return o instanceof List  ((List) o).isEmpty();
+  return o instanceof List?  ((List?) o).isEmpty();
 }
 
 /**
@@ -480,10 +481,10 @@ public class Collections
    * @return an empty parameterized map.
    * @since 1.5
    */
+  @SuppressWarnings(unchecked)
   public static final K,V MapK,V emptyMap()
   {
-    /* FIXME: Could this be optimized? */
-    return new EmptyMapK,V();
+    return (MapK,V) EMPTY_MAP;
   }
 
   /**
@@ -511,9 +512,10 @@ public class Collections
  * There are no entries.
  * @return The empty set.
  */
+    @SuppressWarnings(unchecked)
 public SetMap.EntryK, V entrySet()
 {
-  return EMPTY_SET;
+  return (SetMap.EntryK, V) EMPTY_SET;
 }
 
 // The remaining methods are optional, but provide a performance
@@ -546,7 +548,7 @@ public class Collections
  */
 public boolean equals(Object o)
 {
-  return o instanceof Map  ((Map) o).isEmpty();
+  return o instanceof Map?,?  ((Map?,?) o).isEmpty();
 }
 
 /**
@@ -572,9 +574,10 @@ public class Collections
  * No entries.
  * @return The empty set.
  */
+    @SuppressWarnings(unchecked)
 public SetK keySet()
 {
-  return EMPTY_SET;
+  return (SetK) EMPTY_SET;
 }
 
 /**
@@ -601,9 +604,10 @@ public class Collections
  * Collection, will work. Besides, that's what the JDK uses!
  * @return The empty set.
  */
+    @SuppressWarnings(unchecked)
 public CollectionV values()
 {
-  return EMPTY_SET;
+  return (CollectionV) EMPTY_SET;
 }
 
 /**
@@ -1854,7 +1858,7 @@ public class Collections
 public ListT subList(int from, int to)
 {
   if (from == to  (to == 0 || to == 1))
-    return EMPTY_LIST;
+    return emptyList();
   if (from == 0  to == 1)
 return this;
   if (from  to)
@@ -2480,7 +2484,7 @@ public class Collections
  * @throws ArrayStoreException if the type of any element of the
  * collection is not a subtype of the element type of a.
  */
-    public T T[] toArray(T[] a)
+    public E E[] toArray(E[] a)
 {
   synchronized (mutex)
 {


Regards,
Ivan

Mon, 15 Oct 2012 06:09:57 -0400 (EDT) от Andrew Hughes ahug...@redhat.com:
   







 

Re: [cp-patches] [RFC PATCH 2/4] Optimize emptySet/Map/List() in Collections class.

[Andrew Hughes]

- Original Message -
 From: Ivan Maidanski iv...@mail.ru
 
 2011-07-20  Ivan Maidanski  iv...@mail.ru
 
   * java/util/Collections.java:
   (emptySet(), EmptySet.iterator(), emptyList(), emptyMap(),
   EmptyMap.entrySet(), EmptyMap.keySet(), EmptyMap.values(),
   SingletonList.subList(int, int)): Suppress unchecked warnings.
   (emptySet(), emptyList(), emptyMap()): Don't create new instance
   (use
   the corresponding immutable container instance); remove FIXME.
   (EmptySet.equals(Object), EmptyList.equals(Object),
   EmptyMap.entrySet(), EmptyMap.equals(Object), EmptyMap.keySet(),
   EmptyMap.values(), SingletonList.subList(int, int)): Add generic
   typing.
   (SynchronizedCollection.toArray(T[])): Rename T type to E (to
   suppress compiler warning about type hiding).
 ---
  ChangeLog  |   15 +++
  java/util/Collections.java |   33 +++--
  2 files changed, 34 insertions(+), 14 deletions(-)
 
 diff --git a/ChangeLog b/ChangeLog
 index c0d84cd..5690754 100644
 --- a/ChangeLog
 +++ b/ChangeLog
 @@ -1,5 +1,20 @@
  2011-07-20  Ivan Maidanski  iv...@mail.ru
  
 + * java/util/Collections.java:
 + (emptySet(), EmptySet.iterator(), emptyList(), emptyMap(),
 + EmptyMap.entrySet(), EmptyMap.keySet(), EmptyMap.values(),
 + SingletonList.subList(int, int)): Suppress unchecked warnings.
 + (emptySet(), emptyList(), emptyMap()): Don't create new instance
 (use
 + the corresponding immutable container instance); remove FIXME.
 + (EmptySet.equals(Object), EmptyList.equals(Object),
 + EmptyMap.entrySet(), EmptyMap.equals(Object), EmptyMap.keySet(),
 + EmptyMap.values(), SingletonList.subList(int, int)): Add generic
 + typing.
 + (SynchronizedCollection.toArray(T[])): Rename T type to E (to
 + suppress compiler warning about type hiding).
 +
 +2011-07-20  Ivan Maidanski  iv...@mail.ru
 +
   * native/jni/java-util/java_util_VMTimeZone.c:
   Include jcl.h file.
   (Java_java_util_VMTimeZone_getSystemTimeZoneId): Throw
 diff --git a/java/util/Collections.java b/java/util/Collections.java
 index 828c6ec..e7e7056 100644
 --- a/java/util/Collections.java
 +++ b/java/util/Collections.java
 @@ -120,10 +120,10 @@ public class Collections
 * @return an empty parameterized set.
 * @since 1.5
 */
 +  @SuppressWarnings(unchecked)
public static final T SetT emptySet()
{
 -/* FIXME: Could this be optimized? */
 -return new EmptySetT();
 +return (SetT) EMPTY_SET;
}

This is ugly, but it seems to be the right thing according to the Javadoc:

Implementation note: Implementations of this method need not create a separate 
Set object for each call. Using this method is likely to have comparable cost 
to using the like-named field. (Unlike this method, the field does not provide 
type safety.

  
/**
 @@ -161,6 +161,7 @@ public class Collections
   * @return A non-iterating iterator.
   */
  // This is really cheating! I think it's perfectly valid,
  though.
 +@SuppressWarnings(unchecked)
  public IteratorT iterator()
  {
return (IteratorT) EMPTY_LIST.iterator();
 @@ -196,7 +197,7 @@ public class Collections
   */
  public boolean equals(Object o)
  {
 -  return o instanceof Set  ((Set) o).isEmpty();
 +  return o instanceof Set?  ((Set?) o).isEmpty();
  }
  
  /**
 @@ -288,10 +289,10 @@ public class Collections
 * @return an empty parameterized list.
 * @since 1.5
 */
 +  @SuppressWarnings(unchecked)
public static final T ListT emptyList()
{
 -/* FIXME: Could this be optimized? */
 -return new EmptyListT();
 +return (ListT) EMPTY_LIST;
}
  
/**
 @@ -369,7 +370,7 @@ public class Collections
   */
  public boolean equals(Object o)
  {
 -  return o instanceof List  ((List) o).isEmpty();
 +  return o instanceof List?  ((List?) o).isEmpty();
  }
  
  /**
 @@ -480,10 +481,10 @@ public class Collections
 * @return an empty parameterized map.
 * @since 1.5
 */
 +  @SuppressWarnings(unchecked)
public static final K,V MapK,V emptyMap()
{
 -/* FIXME: Could this be optimized? */
 -return new EmptyMapK,V();
 +return (MapK,V) EMPTY_MAP;
}
  
/**
 @@ -511,9 +512,10 @@ public class Collections
   * There are no entries.
   * @return The empty set.
   */
 +@SuppressWarnings(unchecked)
  public SetMap.EntryK, V entrySet()
  {
 -  return EMPTY_SET;
 +  return (SetMap.EntryK, V) EMPTY_SET;
  }
  
  // The remaining methods are optional, but provide a performance
 @@ -546,7 +548,7 @@ public class Collections
   */
  public boolean equals(Object o)
  {
 -  return o instanceof Map  ((Map) o).isEmpty();
 +  return o instanceof Map?,?  ((Map?,?) o).isEmpty();
  }
  
  /**
 @@ -572,9 +574,10 @@ public class