groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Wagenleitner <john.wagenleit...@gmail.com>
Subject Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of "static" fields should be "synchronized"
Date Wed, 06 Jan 2016 17:50:32 GMT
It does not look like it relies on the runtime type so that I think using
HMC.class instead of getClass() would be ok, and if that is case it might
be a candidate to be made final and initialized.

    private static final MetaClass myMetaClass =
InvokerHelper.getMetaClass(HandleMetaClass.class);

If making it lazy is important then maybe a lazy initialization holder
static nested class could be used and referenced in the getProperty(String)
method when needed.  Both should be thread-safe, the first being close to
what it was doing and the latter deferring the init to even later.  My
guess is the cost of a single call to getMetaClass may not be worth the
added complexity of making it lazily initialized.


On Wed, Jan 6, 2016 at 1:09 AM, Jochen Theodorou <blackdrag@gmx.org> wrote:

> nono, iot is actually good to question things. Having two people review
> things is good, and I must say, I think I rushed a bit here and did misread
> the code (too much task switching and too little time for Groovy these days
> I assume).
>
> So what is myMetaClass for? When I did the review I assumed it is for the
> object we "handle". But that is not the case. Instead this is really the
> meta class for the handle itself, used by the handle itself.
>
> What does it mean then to have this static? Subclasses will not influence
> that and there won't be any per instance meta classes for this. Now I think
> it would be no good to have a per instance meta class on a handle meta
> class... I mean a meta class of a meta class... well...
>
> In fact I now think we should do a different change. If we decide to have
> this static, then I guess the class should actually be final.
>
> And then I would suggest writing a static method to get this meta class,
> which does also do the init and everything wanting to work with the
> myMetaClass should use this method then. This actually moves the init of
> the meta class to an even later point than it is right now, but that is
> fine.
>
> As for concurrency... I think if done this way it is no problem. Multiple
> threads might create multiple meta classes and they are different instances
> and all, but the will do the same, so in the end it does not matter which
> of those is used in the end. The meta class itself is responsible for
> proper synchronization and initializaton of anything that is visible across
> threads, so no external synchronization is required.
>
> what do you guys think?
>
> bye Jochen
>
> Am 06.01.2016 um 00:27 schrieb John Wagenleitner:
>
>> My mistake, I didn't know there was a PR associated that Jochen had
>> already reviewed.  Sorry about that.
>>
>> On Tue, Jan 5, 2016 at 2:45 PM, Pascal Schumacher
>> <pascalschumacher@gmx.net <mailto:pascalschumacher@gmx.net>> wrote:
>>
>>     Yes, I agree the change most likely reduces performance, but
>>     increases thread-safety  (prevent that different Threads may work
>>     with different Metaclasses etc.)
>>
>>     I do not know enough about this area of the code to judge if the
>>     lack of thread-safety is really a concern.
>>
>>     I just merged the pull request because of Jochens +1 vote.
>>
>>
>>     Am 05.01.2016 um 20:31 schrieb John Wagenleitner:
>>
>>>     Not sure but wonder if HandleMetaClass#myMetaClass was static to
>>>     avoid having to perform a lookup for each HMC instance.  Probably
>>>     not a issue but thought I'd bring attention to it.
>>>
>>>     On Tue, Jan 5, 2016 at 10:13 AM, <pascalschumacher@apache.org
>>>     <mailto:pascalschumacher@apache.org>> wrote:
>>>
>>>         Repository: groovy
>>>         Updated Branches:
>>>           refs/heads/master 586a316da -> c5f17abbe
>>>
>>>
>>>         Fixing squid:S2444 - Lazy initialization of "static" fields
>>>         should be "synchronized"
>>>
>>>         <snip>
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/groovy/blob/c5f17abb/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>>
>>> ----------------------------------------------------------------------
>>>         diff --git
>>>         a/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>>         b/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>>         index f421131..9167f5c 100644
>>>         --- a/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>>         +++ b/src/main/org/codehaus/groovy/runtime/HandleMetaClass.java
>>>         @@ -24,7 +24,7 @@ import java.lang.reflect.Method;
>>>
>>>          public class HandleMetaClass extends DelegatingMetaClass {
>>>              private Object object;
>>>         -    private static MetaClass myMetaClass;
>>>         +    private MetaClass myMetaClass;
>>>              private static final Object NONE = new Object();
>>>
>>>              public HandleMetaClass(MetaClass mc) {
>>>
>>>         <snip>
>>>
>>>
>>
>>

Mime
View raw message