[
https://issues.apache.org/jira/browse/TRINIDAD-1460?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12764804#action_12764804
]
Jeanne Waldman commented on TRINIDAD-1460:
------------------------------------------
This is related to TRINIDAD-1594 FileSystemStyleCache doesn't scale due to
over-synchronization
> implement FileSystemStyleCache code review comments
> ---------------------------------------------------
>
> Key: TRINIDAD-1460
> URL: https://issues.apache.org/jira/browse/TRINIDAD-1460
> Project: MyFaces Trinidad
> Issue Type: Improvement
> Components: Skinning
> Reporter: Jeanne Waldman
> Priority: Minor
>
> Code review comments from Blake Sullivan for FileSystemStyleCache:
> in private StyleSheetDocument _getStyleSheetDocument(StyleContext context)
> this part:
> // Otherwise, we create the StyleSheetDocument now
> document = createStyleSheetDocument(context);
> // If we weren't able to create the StyleSheetDocument,
> // use a non-null placeholder
> if (document == null)
> document = _EMPTY_DOCUMENT;
> // Save the document
> if (_document == null)
> _document = document;
> // Re-initialize our Array of namespace prefixes that are in the selectors
> // Re-initialize our Map of short style class names
> _namespacePrefixes = _getNamespacePrefixes(context, _document);
> _shortStyleClassMap = _getShortStyleClassMap(context, _document,
> _namespacePrefixes);
> Should probably be in a synchronized block
> _document, _namespacePrefixes, and _shortStyleClassMap should be volatile.
> comment about not needing to use Hashtable and HashMap being OK is
> incorrect--_createEntry() which assigns values to _cache and _entryCache is
> called without a lock and these Maps are read without a lock. _cache and
> _entryCache should probably be ConcurrentHashMaps.
> private static class Key
> Might as well make the class final as well
> Key.hashCode()--hash algorithm is a little weak, since it doesn't spread the
> bits out enough. Effective Java's version would be better
> Key should calculate its hashCode on initialization because:
> 1) The comment about not needing to synchronize is wrong
> 2) The very first thing we do is perform a get on the key, so there is no
> performance benefit to not calculating the hashCode aggressively
> All fields should be final, making a nice immutable instance, which we
> should document as such.
> The Entry class should also be documented as thread-safe, though, in this
> case, it is only because the List of URIs is implemented as an ArrayList and
> the List isn't modified after the Entry class gets the list. We will need to
> document this restriction unless we actually copy the list of URIs when
> creating the Entry.
> private static class DerivationKey
> Pretty much the same issues as Key--make the class final, make the fields
> final, aggressively calculate the hashCode, fix the hashCode implementation,
> document the class as immutable.
> private DerivationKey _getDerivationKey(
> The whole synchronization thing is bogus--the set of style sheets on the
> StyleSheetDocument is immutable, so _copyIterator() should be removed and the
> function written as:
> // Entries with the same style sheet derivation are compatible.
> // Get the style sheet derivation list.
> Iterator<StyleSheetNode> styleSheetNodes =
> document.getStyleSheets(context);
> // =-= bts note that we could also make a constant private static final
> EMPTY_STYLE_SHEET_NODES =
> // new StyleSheetNode[0]; which would be better.
> StyleSheetNode[] styleSheets = new StyleSheetNode[0];
> // if we have any style sheets, copy them into an array of the correct
> type
> if (styleSheetNodes.hasNext())
> {
> List<StyleSheetNode> styleSheetNodeList =
> CollectionUtils.arrayList(styleSheetNodes);
> styleSheets = styleSheetNodeList.toArray(styleSheets);
> }
> // Create a key out of the style sheet derivation list
> return new DerivationKey(context, styleSheets);
> _getStyleContextResolvedStyles
> replace:
> List<StyleNode> v = new ArrayList<StyleNode>();
> while (e.hasNext())
> v.add(e.next());
> return v.toArray(new StyleNode[v.size()]);
> with
> List<StyleNode> v = CollectionUtils.arrayList(e);
> return v.toArray(new StyleNode[v.size()]);
> dd
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
