Re: RFR: 8343418: Use HashMap instead of Hashtable for CSS.htmlAttrToCssAttrMap

[Andrey Turbanov]

On Thu, 31 Oct 2024 20:36:28 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:

>> If a thread-safe implementation is not needed, it is recommended to use 
>> HashMap instead of legacy synchronized Hashtable. 
>> Map `CSS.htmlAttrToCssAttrMap` is modified only within static initialization 
>> block and then only `get` method is called.
>> 
>> The only subtle difference is that Hashtable doesn't allow `null` keys and 
>> throws NPE from `get` method.
>> I've checked all possible keys which are passed to 
>> `htmlAttrToCssAttrMap.get`.
>> Currently 3 different execution paths are possible:
>> 
>> **1,2**
>> When `HTML.Attribute.BORDER` is passed as a key. It's definitely non-null.
>> 
>> javax.swing.text.html.CSS#translateHTMLToCSS
>>   translateAttribute(HTML.Attribute.BORDER, "1", cssAttrSet);
>>     CSS.Attribute[] cssAttrList = getCssAttribute(key);
>> 
>> 
>> javax.swing.text.html.CSS#translateAttributes
>>    translateAttribute(HTML.Attribute.BORDER, Integer.toString(borderWidth), 
>> cssAttrSet);
>>      CSS.Attribute[] cssAttrList = getCssAttribute(key);`
>> 
>> 
>> **3**
>> When non-null `key` is passed as a key.
>> 
>> javax.swing.text.html.CSS#translateAttributes
>> 
>>     if (name instanceof HTML.Attribute) { // from this `instanceof` we know 
>> that it's non-null
>>          HTML.Attribute key = (HTML.Attribute)name;
>> 
>>       translateAttribute(key, (String) htmlAttrSet.getAttribute(key), 
>> cssAttrSet);
>>         CSS.Attribute[] cssAttrList = getCssAttribute(key);
>> 
>> 
>> ![idea_analyze_dataflow_to_here](https://github.com/user-attachments/assets/48ace4de-6d0a-468e-bb14-b579a193254b)
>> 
>> 
>> I've used HashMap.newHashMap method instead of constructor to avoid resizing 
>> of internal HashMap array.
>
> @turbanoff Based on your explanation, it sounds reasonable to convert 
> CSS.htmlAttrToCssAttrMap from Hashtable to HashMap since synchronization may 
> not be required.
> 
> There are few more Hashtables - `styleConstantToCssMap, 
> htmlValueToCssValueMap, cssValueToInternalValueMap`. Do we consider 
> converting them as well - either to HashMap or ConcurrentHashMap (if 
> thread-safe synchronization is required)?

@honkar-jdk yes. It was my intention to convert them all. I want to go 
one-by-one to make review easier.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/21785#issuecomment-2450779444