changeset 1659:489caa3dfeb5

RT-29975: key to lookup cached styles depended on which parent nodes had parent stylesheets. This led to cache misses because an identical scene-graph would have a different key simply because the node instances were different. Instead, the key can just use the list of parent stylesheets.
author David Grieve<david.grieve@oracle.com>
date Tue, 25 Jun 2013 13:57:45 -0400
parents ff55a1a6dfa5
children cb06314b54b9
files javafx-ui-common/src/com/sun/javafx/css/StyleManager.java
diffstat 1 files changed, 52 insertions(+), 89 deletions(-) [+]
line wrap: on
line diff
--- a/javafx-ui-common/src/com/sun/javafx/css/StyleManager.java	Thu Jun 20 10:13:52 2013 -0700
+++ b/javafx-ui-common/src/com/sun/javafx/css/StyleManager.java	Tue Jun 25 13:57:45 2013 -0400
@@ -136,12 +136,28 @@
         // This list should also be fairly small
         private final RefList<Key> keys;
 
+        final int hash;
+        
         private ParentStylesheetContainer(String fname, Stylesheet stylesheet) {
             this.fname = fname;
+            this.hash = fname != null ? fname.hashCode() : 127;
             this.stylesheet = stylesheet;
             this.parents = new RefList<Parent>();
             this.keys = new RefList<Key>();
         }
+        
+        @Override public int hashCode() { return hash; }
+        @Override public boolean equals(Object obj) {
+            if (this == obj) return true;
+            if (obj == null || obj.getClass() != this.getClass()) {
+                return false;
+            }
+            ParentStylesheetContainer other = (ParentStylesheetContainer)obj;
+            if (this.fname != null ? !this.fname.equals(other.fname) : other.fname != null) {
+                return false;
+            }
+            return true;
+       }
     }
     
     private static class RefList<K> {
@@ -278,7 +294,7 @@
      */
     private Map<WeakReference<Scene>, StylesheetContainer> containerMap;
 
-    /**
+   /**
      * Remove mappings for keys with null referents.
      * Should only be called from get and put.
      * @param map
@@ -723,6 +739,7 @@
     
     private void userAgentStylesheetsChanged() {
         if (defaultContainer != null) {
+            System.err.println("userAgentStylesheetsChanged - blow away defaultContainer");
             //
             // RT-21563 if default ua stylesheet is set, then the default 
             // container must be destroyed so it will be rebuilt. If this is 
@@ -1083,7 +1100,7 @@
         }
 
         private void destroy() {
-            stylesheets.clear();
+           stylesheets.clear();
             clearCaches();
         }
 
@@ -1096,26 +1113,7 @@
         private long nextSmapId() {
             return ++smapCount;
         }
-        //
-        // This int array represents the set of Parent's that have stylesheets.
-        // The indice is that of the Parent's hash code or -1 if 
-        // the Parent has no stylesheet. See also the comments in the Key class.
-        // Note that the array is ordered from root at index zero to the 
-        // leaf Node's parent at index length-1.
-        // 
-        private static int[] getIndicesOfParentsWithStylesheets(Parent parent, int count) {
-            if (parent == null) return new int[count];
-            final int[] indices = getIndicesOfParentsWithStylesheets(parent.getParent(), ++count);
-            // logic elsewhere depends on indices of parents
-            // with no stylesheets being -1
-            if (parent.getStylesheets().isEmpty() == false) {
-                indices[indices.length-count] = parent.hashCode();
-            } else {
-                indices[indices.length-count] = -1;
-            }
-            return indices;
-        }
-        
+
         //
         // recurse so that stylesheets of Parents closest to the root are 
         // added to the list first. The ensures that declarations for 
@@ -1166,40 +1164,35 @@
             
             return list;
         }
-        
+       
         /**
          * Returns a StyleHelper for the given Node, or null if there are no
          * styles for this node.
          */
         private StyleHelper getStyleHelper(Node node) {
-            
+          
             // Populate our helper key with the class name, id, and style class
             // of the node and lookup the associated Cache in the cacheMap
             final String className = node.getClass().getName();
             final String id = node.getId();
             final List<String> styleClass = node.getStyleClass();
             
-            final int[] indicesOfParentsWithStylesheets =  
-                getIndicesOfParentsWithStylesheets(
-                    ((node instanceof Parent) ? (Parent)node : node.getParent()), 0
-                );
-            //
-            // avoid calling gatherParentStylesheets later if we know there
-            // aren't any parent stylesheets. If there are parent stylesheets,
-            // then at least one element of indicesOfParentsWithStylesheets
-            // will be other than -1
-            //
-            boolean hasParentStylesheets = false;
-            for (int n=0; n<indicesOfParentsWithStylesheets.length; n++) {
-                // assignment, not equality here!
-                if (hasParentStylesheets = (indicesOfParentsWithStylesheets[n] != -1)) break;
-            }
+            final Parent parent =
+                (node instanceof Parent)
+                    ? (Parent) node : node.getParent();
             
+            final List<ParentStylesheetContainer> parentStylesheets =
+                        gatherParentStylesheets(parent);
+
+            final boolean hasParentStylesheets =
+                    parentStylesheets != null && parentStylesheets.isEmpty() == false;
+
+           
             key.className = className;
             key.id = id;
             key.styleClass = styleClass;
-            key.indices = hasParentStylesheets ? indicesOfParentsWithStylesheets : null;
-            
+            key.parentStylesheets = hasParentStylesheets ? parentStylesheets : null;
+
             Cache cache = cacheMap.get(key);
 
             // the key is an instance variable and so we need to null the
@@ -1230,17 +1223,9 @@
                 // then the selector impacts its children.
                 boolean impactsChildren = false;
                 
-                final List<ParentStylesheetContainer> parentStylesheets = 
-                    hasParentStylesheets 
-                        ? gatherParentStylesheets(
-                            ((node instanceof Parent) ? (Parent)node : node.getParent())
-                        )
-                        : null;
-                
-                //
                 List<Stylesheet> stylesheetsToProcess = null;
                 
-                if (parentStylesheets == null || parentStylesheets.isEmpty()) {
+                if (hasParentStylesheets == false) {
                     stylesheetsToProcess = stylesheets;
                 } else {
                     
@@ -1365,8 +1350,8 @@
                 // the cacheMap lookup should miss.
                 final int nElements = styleClass.size();
                 newKey.styleClass = new ArrayList<String>(nElements);
-                for(int n=0; n<nElements; n++) newKey.styleClass.add(styleClass.get(n));
-                newKey.indices = hasParentStylesheets ? indicesOfParentsWithStylesheets : null;
+                for(int n=0; n<nElements; n++) newKey.styleClass.add(styleClass.get(n));                
+                newKey.parentStylesheets = key.parentStylesheets;
                 
                 cache = new Cache(rules, pseudoclassStateMask, impactsChildren);
                 cacheMap.put(newKey, cache);
@@ -1496,9 +1481,9 @@
                 Rule rule = rules.get(i);
                 if (rule.applies(node)) {
                     key = key | mask;
-                }
+                        }
                 mask = mask << 1;
-            }
+                    }
             
             final Long keyObj = Long.valueOf(key);            
             if (cache.containsKey(keyObj)) {
@@ -1594,19 +1579,14 @@
         String className;
         String id;
         List<String> styleClass;
+
+        // this will be initialized if a Parent has a stylesheet and will 
+        // hold the string URLs of those Parent stylesheets. If there are 
+        // parent stylesheets involved, then one key equals another if 
+        // the other fields equal _and_ the keys have the same list of 
+        // stylesheets.
+        List<ParentStylesheetContainer> parentStylesheets;
         
-        // this will be initialized if a Parent has a stylesheet and will 
-        // hold the indices of those Parents with stylesheets (the Parent's
-        // hash code is used). If the parent does not have a stylesheet, the 
-        // indice will be -1. When finding the equals Key in the cache lookup,
-        // we only need to check up to the last parent that has a stylesheet.
-        // In other words, if one node is at level n and nearest parent with
-        // a stylesheet is at level m, then the indices match provided 
-        // indices[x] == other.indices[x] for 0 <= x <= m and all other
-        // indices are -1. Thus, [1, -1, 2] and [1, -1, 2, -1, -1] are equivalent
-        // whereas [1, -1, 2] and [1, -1, 2, -1, 3] are not.
-        int[] indices;
-
         @Override
         public boolean equals(Object o) {
             if (this == o) return true;
@@ -1614,32 +1594,15 @@
                 Key other = (Key)o;
                 boolean eq =
                         className.equals(other.className)
-                    && (   (indices == null && other.indices == null)
-                        || (indices != null && other.indices != null)
-                       )
                     && (   (id == null && other.id == null)
                         || (id != null && id.equals(other.id))
                        )
                     && (   (styleClass == null && other.styleClass == null)
-                        || (styleClass != null && styleClass.containsAll(other.styleClass))
+                        || (styleClass != null && styleClass.containsAll(other.styleClass)))
+                    && (   (parentStylesheets == null && other.parentStylesheets == null)
+                        || (parentStylesheets != null && parentStylesheets.equals(other.parentStylesheets))
                        );
-                
-                if (eq && indices != null) {
-                    final int max = Math.min(indices.length, other.indices.length);
-                    for (int x = 0; eq && (x < max); x++) {
-                        eq = indices[x] == other.indices[x];
-                    }
-                    if (eq) {
-                        // ensure the remainder are -1
-                        for(int x=max; eq && x<indices.length; x++) {
-                            eq = indices[x] == -1;
-                        }
-                        for(int x=max; eq && x<other.indices.length; x++) {
-                            eq = other.indices[x] == -1;
-                        }
-                    }
-                }
-                return eq;
+               return eq;
             }
             return false;
         }
@@ -1649,13 +1612,13 @@
             int hash = className.hashCode();
             hash = 31 * (hash + ((id == null || id.isEmpty()) ? 1231 : id.hashCode()));
             hash = 31 * (hash + ((styleClass == null || styleClass.isEmpty()) ? 1237 : styleClass.hashCode()));
-            if (indices != null) hash = 31 * (hash + Arrays.hashCode(indices));
+            if (parentStylesheets != null) hash = 31 * (hash + parentStylesheets.hashCode());
             return hash;
         }
 
         @Override
         public String toString() {
-            return "Key ["+className+", "+String.valueOf(id)+", "+String.valueOf(styleClass)+"]";
+            return "Key ["+className+", "+String.valueOf(id)+", "+String.valueOf(styleClass)+", "+String.valueOf(parentStylesheets)+"]";
         }
         
     }