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.wallnoe...@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.athijegannat...@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.ha...@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.ha...@oracle.com>: >>>>>>>>> >>>>>>>>> Hi Attila, >>>>>>>>> >>>>>>>>> lower-case thumbs up! >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> >>>>>>>>> Michael >>>>>>>>> >>>>>>>>>> Am 17.09.2015 um 16:14 schrieb Attila Szegedi >>>>>>>>>> <attila.szeg...@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 >>>>>>>> >>>> >>> >> >
