Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-6276988

(coll) EnumSet is not "Extremely compact" as claimed

    Details

    • Subcomponent:
    • Resolved In Build:
      b78
    • CPU:
      generic, x86
    • OS:
      generic, windows_2000
    • Verification:
      Verified

      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

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                sseligmasunw Scott Seligman (Inactive)
                Reporter:
                tyao Ting-Yun Ingrid Yao (Inactive)
              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Imported:
                  Indexed: