Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

Hi Sergey/Semyon,   I have pushed webrev01 which simply fixes the bug as reported, and created a new issue JDK-8199400 to address the point of changing Hashtable to HashMap. Once we are clear about the changes, we can push them with this bug.   Thanks, Krishna   From: Krishna Addepalli

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

Hi Sergey,   I understand the behavior change of raising an exception vs quietly returning null. As per my understanding, this happens only when someone passes a null for the locale key right? If so, how about explicitly checking for null, and throwing exception?   @Semyon,   I think we

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

I just noticed that null returning scenario only possible when locale independent state name is null but the latter should never happen. So we simply may ignore this null/NPE worry. --Semyon On 3/7/18 11:17 AM, semyon.sadet...@oracle.com wrote: On 3/7/18 10:40 AM, Sergey Bylokhov wrote:

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

On 07/03/2018 11:17, semyon.sadet...@oracle.com wrote: I found nothing related to the behavior, only some conclusion about possible performance difference. So you agree? Agreed with what? Unrelated to the current fix and unproven possible performance change in rare situation is not outweigh

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

On 07/03/2018 10:21, semyon.sadet...@oracle.com wrote: That's not ok, since we change a behavior without discussion that the benefits of change outweigh the disadvantages, when we implemented some unrelated fix. That's not true. You may find the discussion in the current thread. Fill free to

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

On 07/03/2018 09:54, semyon.sadet...@oracle.com wrote: On 3/7/18 8:53 AM, Sergey Bylokhov wrote: Hi, Krishna. The .02 version of the fix have changed the behavior of toDisplayString() method. Before the fix it throws NPE for key=null, after the fix it returns null. That's ok. That's not

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

Thanks for the review Semyon! -Original Message- From: Semyon Sadetsky Sent: Wednesday, March 7, 2018 9:27 PM To: Krishna Addepalli ; Philip Race Cc: swing-dev@openjdk.java.net Subject: Re:

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

+1 --Semyon On 3/7/18 5:28 AM, Krishna Addepalli wrote: Hi Semyon, Modified the code as per your suggestions. Here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8197785/webrev02 Thanks, Krishna -Original Message- From: Semyon Sadetsky Sent: Tuesday, March 6, 2018 11:46

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

Hi Semyon, Modified the code as per your suggestions. Here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8197785/webrev02 Thanks, Krishna -Original Message- From: Semyon Sadetsky Sent: Tuesday, March 6, 2018 11:46 PM To: Phil Race ; Krishna

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

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.

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

I originally thought you were referring to the Hashtable in the test ? 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

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

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

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

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"? Besides, whether we use HashMap /Hashtable, the fix for this particular bug still remains. As for changing (if that is

Re: [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

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

Hi Krishna, Is there any reason to use Hashmap. It seems the synchronization is surplus here and its better to use HashMap. --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

Re: [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString

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

Re: [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

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

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/ As the bug description suggests, AccessibleBundle.loadResourceBundle has the line "table.contains" which causes it to