<Swing Dev> [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

Tue Mar 6 17:45:39 UTC 2018

On 03/05/2018 12:49 PM, Phil Race wrote:

> I originally thought you were referring to the Hashtable in the test ?
No. I meant Hashtable created in line 151.
> That really doesn't matter.
> If you are referring to the use of hashtable in the JDK class then I
> think it is safer to leave it as is. Hashtable is provably safe here, and
> for HashMap you'd need to ensure that concurrent read and write
> are safe. The HashMap is created only once but may be updated
> more than once, even if it is not likely in real world use (multiple 
> locales in use).
Can you point to place where this Hashmap is updated other then where it 
is initialized?

> The performance cost here is negligible, probably not measurable in
> real world code where there is no contention for the lock
This requires evidence. You need to know the load characteristic to 
state that. If collisions are proven to be rare the optimistic locking 
is preferable. But still, I don't see any grounds to have any collisions 
since these are read-only data. Synchronization may degrade performance 
a lot and may be a source for deadlocks so it's always better to avoid it.

--Semyon
>
> -phil.
>
> On 03/05/2018 11:21 AM, Semyon Sadetsky wrote:
>>
>> On 03/05/2018 10:41 AM, Krishna Addepalli wrote:
>>
>>> Hi Semyon,
>>>
>>> Thank you for the review. I don’t see any reason why HashMap can’t 
>>> be used here. But could you clarify what you meant by 
>>> “synchronization is surplus here”?
>>>
>> Sure. Hashtable is synchronized. And since the locale bundle is 
>> created once now and then is only read there is no need to have extra 
>> thread access synchronization.
>>>
>>> Besides, whether we use HashMap /Hashtable, the fix for this 
>>> particular bug still remains.
>>>
>>> As for changing (if that is necessary), I think we should address it 
>>> in a separate bug?
>>>
>> It is not unrelated completely since you are fixing a performance 
>> issue. I suggest to fix it for one since it is easy.
>>
>> --Semyon
>>>
>>> Thanks,
>>>
>>> Krishna
>>>
>>> *From:*Semyon Sadetsky
>>> *Sent:* Monday, March 5, 2018 10:39 PM
>>> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>; Philip Race 
>>> <philip.race at oracle.com>
>>> *Cc:* swing-dev at openjdk.java.net
>>> *Subject:* Re: <Swing Dev> 
>>> [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload 
>>> the ResourceBundle for every call to toDisplayString
>>>
>>> On 03/05/2018 09:00 AM, Semyon Sadetsky wrote:
>>>
>>>     Hi Krishna,
>>>
>>>     Is there any reason to use Hashmap. It seems the synchronization
>>>     is surplus here and its better to use HashMap.
>>>
>>> I meant Hashtable. Sorry.
>>>
>>> --Semyon
>>>
>>>     --Semyon
>>>
>>>     On 03/05/2018 05:39 AM, Krishna Addepalli wrote:
>>>
>>>         Hi Phil
>>>
>>>         Thank you for the review.
>>>
>>>         I have checked the accessibility package, and found that
>>>         contains is used with a Vector of AccessibleState objects in
>>>         the file AccessibleStateset.
>>>
>>>         However in AccessibilityBundle, the table is used to store a
>>>         set of values associated with a particular locale, and going
>>>         by the context, I find little reason that using contains is
>>>         intentional here.
>>>
>>>         And regarding the testcase, the thought of making it
>>>         headless crossed my mind while writing it, but I was not
>>>         sure if just creating a Button is allowed in headless mode.
>>>
>>>         Fortunately, I modified the testcase enough that, no swing
>>>         widgets need to be created, and we can safely run the test
>>>         in headless mode.
>>>
>>>         Here is the new webrev:
>>>         http://cr.openjdk.java.net/~kaddepalli/8197785/webrev01/
>>>         <http://cr.openjdk.java.net/%7Ekaddepalli/8197785/webrev01/>
>>>
>>>         Thanks,
>>>
>>>         Krishna
>>>
>>>         *From:*Philip Race
>>>         *Sent:* Saturday, March 3, 2018 9:44 PM
>>>         *To:* Krishna Addepalli <krishna.addepalli at oracle.com>
>>>         <mailto:krishna.addepalli at oracle.com>
>>>         *Cc:* swing-dev at openjdk.java.net
>>>         <mailto:swing-dev at openjdk.java.net>
>>>         *Subject:* Re: <Swing Dev>
>>>         [11][JDK-8197785]javax.accessibility.AccessibilityBundle
>>>         will reload the ResourceBundle for every call to toDisplayString
>>>
>>>         The fix is straightforward and I've seen this bug pattern
>>>         before.
>>>         In fact there may even have been a sweep for uses of
>>>         contains() to make sure it was as intended,
>>>         but if so it wasn't thorough enough.
>>>         But I'm wondering why the test extends Button and not JButton ?
>>>         I'm then further wondering if it could then be made headless
>>>         .. ie no need to create or display a Frame ?
>>>
>>>         -phil.
>>>
>>>         On 3/3/18, 6:30 AM, Krishna Addepalli wrote:
>>>
>>>             Hi All,
>>>
>>>             Please review a simple fix for JDK-8197785:
>>>             https://bugs.openjdk.java.net/browse/JDK-8197785
>>>
>>>             Webrev:
>>>             http://cr.openjdk.java.net/~kaddepalli/8197785/webrev00/
>>>             <http://cr.openjdk.java.net/%7Ekaddepalli/8197785/webrev00/>
>>>
>>>             As the bug description suggests,
>>>             AccessibleBundle.loadResourceBundle has the line
>>>             “table.contains” which causes it to reload the resource
>>>             bundle for each call.
>>>
>>>             Changing it to “table.containsKey” fixes the problem.
>>>
>>>             Thanks,
>>>
>>>             Krishna
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180306/c7c8145d/attachment.html>