[jira] [Commented] (COLLECTIONS-803) CaseInsensitiveMap prevent duplicate key conversion on put

["Alex Herbert (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/COLLECTIONS-803?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17705133#comment-17705133 ] 

Alex Herbert commented on COLLECTIONS-803:
------------------------------------------

This is a case where the implementation in the AbstractHashedMap is not optimal. Ideally the {{convertKey}} method should only be invoked once on any argument to a public method. Thus the internal {{addMapping}} function should have been written to allow passing an already converted key. However there are implementations in the library that use this function: {{LRUMap}} overrides the method and {{MultiKeyMap}} extensively calls it adding new MultiKey objects. Thus changing the behaviour of {{addMapping}} to require {{convertKey}} to be called would be a breaking _behavioural_ change and may require modification of those classes. Note that the default implementation of {{convertKey}} key trivially changes {{null}} to a marker object. Only derived classes that override {{convertKey}} will be affected.

The path of least resistance without polluting the API with more methods is to reimplement the {{AbstractHashedMap.put}} method in {{CaseInsensitiveMap}} to pass the converted key to {{addMapping}} and override the {{createEntry}} method to know that the key has already been converted. This override will be a _behavioural_ change for any class extending the CaseInsensitiveMap. This is not done within the library.

Alternatives are to create new methods to perform this behaviour as private (thus not overridable or callable by any child classes), or pollute the API with protected methods to perform this behaviour. This will preserve functionality of any derived classes that use the existing {{createEntry}} method, or {{addMapping}} method.

The decision is to weigh the cost of a _behavioural_ change to derived classes (which are outside of this library), verses the performance improvement for all users of the CaseInsensitiveMap.

The implementation that hides method implementations privately does not allow simple extension. For example a child class that wished to use a decorated HashEntry<K, V> to store addition information with each entry would have to override the {{put}} method to call a reimplementation of the {{addMapping}} function. So I suggest the best approach would be to implement the functionality by overriding the minimum of existing methods. I believe this would be {{put}} and {{{}createEntry{}}}. These should be documented to be overridden to prevent duplicate calls to {{{}convertKey{}}}. The {{addMapping}} method would require documentation to indicate that the key is expected to be pre-converted by {{{}convertKey{}}}, e.g.
{code:java}
/**
 * {@inheritDoc}
 * 
 * <p>Note: This method should be called with a {@code key} that has been pre-converted
 * using {@link #convertKey(Object)}.
 */
@Override
protected void addMapping(int hashIndex, int hashCode, K key, V value) {
    super.addMapping(hashIndex, hashCode, key, value);
}
{code}


> CaseInsensitiveMap prevent duplicate key conversion on put
> ----------------------------------------------------------
>
>                 Key: COLLECTIONS-803
>                 URL: https://issues.apache.org/jira/browse/COLLECTIONS-803
>             Project: Commons Collections
>          Issue Type: Improvement
>          Components: Map
>    Affects Versions: 4.4
>            Reporter: Simulant
>            Priority: Minor
>              Labels: performance
>          Time Spent: 1h 50m
>  Remaining Estimate: 0h
>
> When adding a new item into a {{CaseInsensitiveMap}} the {{convertKey(key)}} method is called twice, once in the {{put(key, value)}} method and second in the {{createEntry(next, hashCode, key, value)}} method. The result could be re-used resulting in a better performance.
> Depending on the {{toString()}} implementation of the key and the resulting length of the key before the lower case conversion the operation can get expensive and should not be called twice, as the {{CaseInsensitiveMap}} overwrites the {{convertKey(key)}} method and makes it more expensive and depending on the input unlike in the implementation of the {{AbstractHashedMap}}.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)