Review request for JDK-8136700: Make sure Context.anonymousHostClasses doesn't grow unbounded

[Attila Szegedi]

Well, you’re right in the sense that the method is, in fact, manipulating a non-threadsafe data structure, and it is probably better to have it explicitly synchronized than relying on the fact that all its callers are synchronized too. I mean, it’s private method calling private method, so we can have a closed-world assumption that the synchronization is unnecessary, but maybe this is one of those better safe than sorry situations. I’ll just hope that this is one of those assumptions that HotSpot can prove and optimize away :-)

Do I have the +1 if I add the synchronized keyword?

> On Sep 18, 2015, at 4:13 PM, Hannes Wallnoefer <hannes.wallnoefer at oracle.com> wrote:
> 
> Am 2015-09-18 um 15:59 schrieb Sundararajan Athijegannathan:
>> I presume it is for code clarity sake? That is a private method in that class called only from the other synchronized method. How about just adding an assert using Thread.holdsLock?
> 
> Well it's mostly as a guard for potential changes down the road. Nothing says "this method needs to be synchronized" better than just making it synchronized IMO. :)
> 
> Hannes
> 
>> -Sundar
>> 
>> On 9/18/2015 7:14 PM, Hannes Wallnoefer wrote:
>>> Shouldn't the methods modifying the map also be synchronized, even if we know they are (currently) only called from a synchronized method?
>>> 
>>> Otherwise looks good.
>>> 
>>> Hannes
>>> 
>>> Am 2015-09-18 um 14:05 schrieb Sundararajan Athijegannathan:
>>>> +1
>>>> 
>>>> On 9/18/2015 3:24 PM, Attila Szegedi wrote:
>>>>>> On Sep 18, 2015, at 6:22 AM, Sundararajan Athijegannathan <sundararajan.athijegannathan at oracle.com> wrote:
>>>>>> 
>>>>>> * Context.compile is synchronized and so there is no need for ConcurrentHashMap + concurrency related comment.
>>>>> You’re right, I removed them. I updated webrev in-place.
>>>>> 
>>>>>> * Map.remove(Object, Object) is used which is since jdk 1.8. If we have to backport this change to jdk8u (where jdk7 is used as JAVA_HOME to build/test), we've to take care of this.
>>>>> If we backport, this will show up as a compile error, so we’ll adjust then. I’d rather run another round of a short review then than have a less efficient construct in 9 codebase.
>>>>> 
>>>>> Thanks,
>>>>>   Attila.
>>>>> 
>>>>>> -Sundar
>>>>>> 
>>>>>> On 9/17/2015 9:07 PM, Attila Szegedi wrote:
>>>>>>> Indeed, that’s correct. A new webrev is available for review at <http://cr.openjdk.java.net/~attila/8136700/webrev.01.jdk9> that manually handles removal of cleared entries.
>>>>>>> 
>>>>>>> Attila.
>>>>>>> 
>>>>>>>> On Sep 17, 2015, at 4:29 PM, Michael Haupt <michael.haupt at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> Hi again,
>>>>>>>> 
>>>>>>>> no sorry; I have to revoke this as I forgot weak sets aren't weak in the way they have to be in this case.
>>>>>>>> 
>>>>>>>> Best,
>>>>>>>> 
>>>>>>>> Michael
>>>>>>>> 
>>>>>>>>> Am 17.09.2015 um 16:18 schrieb Michael Haupt <michael.haupt at oracle.com>:
>>>>>>>>> 
>>>>>>>>> Hi Attila,
>>>>>>>>> 
>>>>>>>>> lower-case thumbs up!
>>>>>>>>> 
>>>>>>>>> Best,
>>>>>>>>> 
>>>>>>>>> Michael
>>>>>>>>> 
>>>>>>>>>> Am 17.09.2015 um 16:14 schrieb Attila Szegedi <attila.szegedi at oracle.com>:
>>>>>>>>>> 
>>>>>>>>>> Please review JDK-8136700 "Make sure Context.anonymousHostClasses doesn't grow unbounded" at <http://cr.openjdk.java.net/~attila/8136700/webrev.jdk9> for <https://bugs.openjdk.java.net/browse/JDK-8136700>
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> Attila.
>>>>>>>> -- 
>>>>>>>> 
>>>>>>>> <http://www.oracle.com/>
>>>>>>>> Dr. Michael Haupt | Principal Member of Technical Staff
>>>>>>>> Phone: +49 331 200 7277 | Fax: +49 331 200 7561
>>>>>>>> Oracle Java Platform Group | LangTools Team | Nashorn
>>>>>>>> Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
>>>>>>>> <http://www.oracle.com/commitment>    Oracle is committed to developing practices and products that help protect the environment
>>>>>>>> 
>>>> 
>>> 
>> 
>