JDK-6276988 : (coll) EnumSet is not "Extremely compact" as claimed
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util:collections
  • Affected Version: 6
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: generic,windows_2000
  • CPU: generic,x86
  • Submitted: 2005-05-27
  • Updated: 2012-10-08
  • Resolved: 2006-03-23
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 6
6 b78Fixed
Related Reports
Relates :  
Relates :  
Description
FULL PRODUCT VERSION :
java version "1.5.0_02"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_02-b09)
Java HotSpot(TM) Client VM (build 1.5.0_02-b09, mixed mode)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows XP [Version 5.1.2600]

A DESCRIPTION OF THE PROBLEM :
java.util.EnumSet claims "This representation is extremely compact and efficient. The space and time performance of this class should be good enough to allow its use as a high-quality, typesafe alternative to traditional int-based "bit flags."

However, in the implementation there is a Enum[] universe field containing all the Enum constants of the particular Enum type the set holds. This array is always a new clone of the one held by java.lang.Class for use in its getEnumConstants() method.

Therefore an EnumSet of an enum with say 64 values, has both a long to store the set, AND a Enum[64] array holding references to each of the 64 enum constants.

This is hardly "compact" let alone "extremely compact"

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Observed by browsing source code.

Confirmed via the attached source code

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
I would expect the universe arrays for two EnumSets of the same Enum type to share their universe array.

The test class below when run should print the String

"The two EnumSets share the same universe arrays"
ACTUAL -
The test class output is

V:\>java -cp . EnumSetSize
The two EnumSets don't share the same universe arrays

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
import java.util.EnumSet;
import java.lang.reflect.Field;
class EnumSetSize {
    public static void main(String[] args) throws Exception {
        EnumSet<Thread.State> s1 = EnumSet.noneOf(Thread.State.class);
        EnumSet<Thread.State> s2 = EnumSet.noneOf(Thread.State.class);
        Field universeField = EnumSet.class.getDeclaredField("universe");
        universeField.setAccessible(true);
        Enum[] s1universe = (Enum[])universeField.get(s1);
        Enum[] s2universe = (Enum[])universeField.get(s2);
        System.out.format("The two EnumSets %sshare the same universe arrays%n",
            s1universe != s2universe ? "don't " : "");
    }
}

---------- END SOURCE ----------
###@###.### 2005-05-27 03:03:20 GMT
Suggested fix by community member ###@###.###

A DESCRIPTION OF THE FIX :
This patch permits to share a unique enum values array
for al EnumSet create on a same class.

Until now, EnumSet implementation (exactly static method noneOf())
ask the class to obtain the array of enum values using the method
java.lang.Class.getEnumConstant(). Because this method is called by
java.util.EnumSet this method must be public.
Because getEnumConstant() is public, the code
make defensive clone() of the array.
So EnumSet don't share the same array but a clone one, this is inefficient.

The idea of the patch is to provide a special access from EnumSet to
Class.getEnumConstant() that avoid to cloning the array.
Futhermore, i make small changes to avoid another cloning in
Class.enumConstantDirectory().

This patch use the already existant sun.misc.SharedSecret to permit
to access to a package-private version of GetEnumConstants()
named getEnumValues() in the patch.

So the patch :
  - add a method Class.getEnumValues() that copy/paste the code of getEnumConstant()
    without the cloning() the array. I've generify the create of the PrivilegedAction too.

  - rewrite getEnumConstant() to use getEnumValues()

  - rewrite enumConstantDirectory() to use getEnumValues() instead of getEnumConstant()
    and make a minor tweak to this method. The current version of the code
    make a cast for each enum values, i have replaced it by one cast on the array.
 
  - add a method getEnumValues() in the interface sun.misc.JavaLangAccess,
    the signature of the method is slightly different from the one of java.lang.Class
    because we know here that only enum class can call this method.

  -  provide an implementation of this method (getEnumValues) in the anonymous
    class of java.lang.System that provide an implementation  of
    sun.misc.JavaLangAccess

  - rewrite the static method EnumSet.noneOf() to use getEnumValues() via
    SharedSecret instead of using getEnumConstants()

Some collateral remarks :
  - the evaluation of bug 6352179 is wrong. There is two different version
    of valueOf() method in an enum, one static declared in the java.lang.Enum
    that take a class and a name and another generated by the compiler
    that just takes a name. The former use a hashtable declared in
    java.lang.Class but the later iterate over the array to find the enum
   by its name and don't use a hashtable. The bug was reported on the later
   but the evaluation talks about the former.
   The bug must be re-open and re-categorized as a compiler bug.
  - bug 6352179 could be easily corrected by adding a method
    getEnumConstant(String name) in java.lang.Class as requested in
    bug 5034509. getEnumConstant(String name) could be implemented
   by delegating to  enumConstantDirectory().

JDK version :
java version "1.6.0-rc"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.6.0-rc-b62)
Java HotSpot(TM) Client VM (build 1.6.0-rc-b62, mixed mode, sharing)

--- Class.java.old	Sun Dec 04 00:34:16 2005
+++ Class.java	Sun Dec 04 00:44:39 2005
@@ -2875,28 +2875,44 @@
      * @since 1.5
      */
     public T[] getEnumConstants() {
-	if (enumConstants == null) {
-	    if (!isEnum()) return null;
-	    try {
-		final Method values = getMethod("values");
+      T[] values=getEnumValues();
+      if (values==null)
+        return null;
+	  return values.clone();
+    }
+    
+    /**
+     * Returns the elements of this enum class or null if this
+     * Class object does not represent an enum type.
+     * This package-private method is used internally by
+     * EnumSet via sun.misc.SharedSecret.getJavaLangAccess().getEnumValues().
+     *
+     * @since 1.6
+     */
+    T[] getEnumValues() {
+        if (enumValues == null) {
+            if (!isEnum()) return null;
+            try {
+                final Method values = getMethod("values");
                 java.security.AccessController.doPrivileged
-                    (new java.security.PrivilegedAction() {
-                            public Object run() {
-                                values.setAccessible(true);
-                                return null;
-                            }
-                        });
-		enumConstants = (T[])values.invoke(null);
-	    }
-	    // These can happen when users concoct enum-like classes
-	    // that don't comply with the enum spec.
-	    catch (InvocationTargetException ex) { return null; }
-	    catch (NoSuchMethodException ex) { return null; }
-	    catch (IllegalAccessException ex) { return null; }
-	}
-	return enumConstants.clone();
+                (new java.security.PrivilegedAction<Object>() {
+                    public Object run() {
+                        values.setAccessible(true);
+                        return null;
+                    }
+                });
+                enumValues = (T[])values.invoke(null);
+            }
+            // These can happen when users concoct enum-like classes
+            // that don't comply with the enum spec.
+            catch (InvocationTargetException ex) { return null; }
+            catch (NoSuchMethodException ex) { return null; }
+            catch (IllegalAccessException ex) { return null; }
+        }
+        return enumValues;
     }
-    private volatile transient T[] enumConstants = null;
+    
+    private volatile transient T[] enumValues;
 
     /**
      * Returns a map from simple name to enum constant.  This package-private
@@ -2907,18 +2923,18 @@
      */
     Map<String, T> enumConstantDirectory() {
 	if (enumConstantDirectory == null) {
-            T[] universe = getEnumConstants();  // Does unnecessary clone
+            T[] universe = getEnumValues();
             if (universe == null)
                 throw new IllegalArgumentException(
                     getName() + " is not an enum type");
-            Map<String, T> m = new HashMap<String, T>(2 * universe.length);
-            for (T constant : universe)
-                m.put(((Enum)constant).name(), constant);
+            HashMap<String, T> m = new HashMap<String, T>(2 * universe.length);
+            for (Enum<?> constant: (Enum<?>[])universe)
+                m.put(constant.name(), (T)constant);
             enumConstantDirectory = m;
         }
         return enumConstantDirectory;
     }
-    private volatile transient Map<String, T> enumConstantDirectory = null;
+    private volatile transient HashMap<String, T> enumConstantDirectory;
 
     /**
      * Casts an object to the class or interface represented

--- EnumSet.java.old	Sun Dec 04 00:35:12 2005
+++ EnumSet.java	Sat Dec 03 19:26:15 2005
@@ -87,7 +87,7 @@
      * @throws NullPointerException if <tt>elementType</tt> is null
      */
     public static <E extends Enum<E>> EnumSet<E> noneOf(Class<E> elementType) {
-        Enum[] universe = elementType.getEnumConstants();
+        Enum[] universe=sun.misc.SharedSecrets.getJavaLangAccess().getEnumValues(elementType);
         if (universe == null)
             throw new ClassCastException(elementType + " not an enum");
 

--- JavaLangAccess.java.old	Sun Dec 04 00:34:41 2005
+++ JavaLangAccess.java	Sat Dec 03 19:15:44 2005
@@ -29,4 +29,9 @@
 
     /** Set thread's blocker field. */
     void blockedOn(Thread t, Interruptible b);
+    
+    /**
+     * Returns the elements of this enum class.
+     */
+    <T extends Enum<T>> T[] getEnumValues(Class<T> klass);
 }
 
--- System.java.old	Sun Dec 04 00:34:31 2005
+++ System.java	Sun Dec 04 00:47:32 2005
@@ -1131,6 +1131,9 @@
             public void blockedOn(Thread t, Interruptible b) {
                 t.blockedOn(b);
             }
+            public <T extends Enum<T>> T[] getEnumValues(Class<T> klass) {
+                return klass.getEnumValues();
+            }
         });
     }
 
 
R����mi Forax

JUnit TESTCASE :
import java.lang.reflect.Field;
import java.util.EnumSet;

/**
 * @author remi
 */
public class TestEnumSet {
    enum Test {
      one,
      two,
      three,
      four,
      five,
      six,
      seven,
      eight,
      nine,
      ten
    }
    
    private static void testEnumSet() {
      EnumSet<Test> noneSet=EnumSet.noneOf(Test.class);
      EnumSet<Test> allSet=EnumSet.allOf(Test.class);
      
      //System.out.println(noneSet);
      //System.out.println(allSet);
      
      if (getUniverse(noneSet)!=getUniverse(allSet))
        throw new AssertionError("enum set doesn't share universe");
    }
    
    private static <E extends Enum<E>> Enum<E>[] getUniverse(EnumSet<E> set) {
        try {
            return (Enum<E>[])field.get(set);
        } catch (IllegalAccessException e) {
            throw new AssertionError(e);
        }
    }
    
    private static final Field field;
    static {
        try {
            field = EnumSet.class.getDeclaredField("universe");
        } catch (NoSuchFieldException e) {
          throw new AssertionError(e);
        }
        field.setAccessible(true);
    }
    
    public static void main(String[] args) {
      testEnumSet();
    }

}


FIX FOR BUG NUMBER:
6276988

Comments
EVALUATION EnumMap has the same issue, with the same fix.
13-03-2006

EVALUATION delete
13-12-2005

EVALUATION Contribution-Forum:https://jdk-collaboration.dev.java.net/servlets/ProjectForumMessageView?forumID=1463&messageID=10491
06-12-2005

EVALUATION Description is correct. The EnumSet implementation calls Class.getEnumConstants to initialize its "universe". That method does cache its internal "enumConstants" array, but it defensively clones this array (as it must) before returning it to a client. Alas, there are no immutable arrays. The most straightforward fix is a package-private entrypoint in Class that returns the cached (and uncloned) "enumConstants" array, for use within the EnumSet implementation. ###@###.### 2005-05-27 18:48:00 GMT A package private entrypoint in java.lang will not be callable from java.util. ###@###.### 2005-05-27 21:32:05 GMT There are ways of getting at package private members from within classes loaded by the bootstrap class loader. Admittedly they are ugly. Another approach would be maintaining a static hash map in EnumSet. But you'd want to use weak keys, and since the values in the map would reference the keys, this might not work out. ###@###.### 2005-05-27 22:35:21 GMT The map must also be thread safe. ###@###.### 2005-05-27 23:10:44 GMT Responding to an SDN comment: Actually, I think you'd want to use weak values not weak keys, the keys would be Class. If the key is a Class that's not weakly referenced (or the like), that Class can never be unloaded by the VM. ###@###.### 2005-06-01 20:31:25 GMT And another: I think you'd want to use weak values as well as weak keys It's true that weak keys alone won't solve the problem (didn't mean to imply otherwise), but making the values weak as well goes too far. The "weakened" values wouldn't have any strong references at all, so could be collected too quickly. Sharing the already-cached value in the Class seems more promising. ###@###.### 2005-06-04 06:26:12 GMT
27-05-2005

WORK AROUND The clone(), complementOf(EnumSet) and copyOf(EnumSet) methods each create a new EnumSet that shares the universe value with the parent that was cloned. The workaround is to create a single empty EnumSet for each enum, and use one of these methods to create new EnumSets from that one, rather than using the other mechanisms which create bloated EnumSets. ###@###.### 2005-05-27 03:08:34 GMT
27-05-2005