[
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)
