You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@groovy.apache.org by John Wagenleitner <jo...@gmail.com> on 2016/01/05 20:31:39 UTC
Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
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, <pa...@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>
Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
Posted by John Wagenleitner <jo...@gmail.com>.
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 <bl...@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 <ma...@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
>>> <ma...@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>
>>>
>>>
>>
>>
Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
Posted by Jochen Theodorou <bl...@gmx.org>.
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 <ma...@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
>> <ma...@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>
>>
>
>
Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
Posted by Pascal Schumacher <pa...@gmx.net>.
No need to apologize.
It's good that you review changes and point out potential problems. :)
Thanks!
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 <ma...@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
>> <ma...@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>
>>
>
>
Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
Posted by John Wagenleitner <jo...@gmail.com>.
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 <pa...@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, <pa...@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>
>
>
>
Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
Posted by Pascal Schumacher <pa...@gmx.net>.
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
> <ma...@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>
>