You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by pa...@apache.org on 2016/01/05 19:13:09 UTC
groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
Repository: groovy
Updated Branches:
refs/heads/master 586a316da -> c5f17abbe
Fixing squid:S2444 - Lazy initialization of "static" fields should be "synchronized"
Project: http://git-wip-us.apache.org/repos/asf/groovy/repo
Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/c5f17abb
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/c5f17abb
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/c5f17abb
Branch: refs/heads/master
Commit: c5f17abbe663577f7f05aa83aa7c79efc38784f0
Parents: 586a316
Author: Kirill Vlasov <ki...@devfactory.com>
Authored: Wed Dec 30 11:16:11 2015 +0500
Committer: pascalschumacher <pa...@gmx.net>
Committed: Tue Jan 5 19:12:09 2016 +0100
----------------------------------------------------------------------
src/examples/swing/binding/caricature/JCaricature.java | 2 +-
src/main/org/codehaus/groovy/runtime/HandleMetaClass.java | 2 +-
.../groovy/runtime/metaclass/MetaClassRegistryImpl.java | 2 +-
src/main/org/codehaus/groovy/tools/shell/util/Logger.java | 8 ++++++--
src/main/org/codehaus/groovy/vmplugin/v7/IndyInterface.java | 9 ++++++---
5 files changed, 15 insertions(+), 8 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/groovy/blob/c5f17abb/src/examples/swing/binding/caricature/JCaricature.java
----------------------------------------------------------------------
diff --git a/src/examples/swing/binding/caricature/JCaricature.java b/src/examples/swing/binding/caricature/JCaricature.java
index 1707df5..67b557c 100644
--- a/src/examples/swing/binding/caricature/JCaricature.java
+++ b/src/examples/swing/binding/caricature/JCaricature.java
@@ -42,7 +42,7 @@ import javax.swing.JPanel;
* @author sky
*/
public class JCaricature extends JPanel {
- private static Map/*<String,Image>*/ imageMap;
+ private Map/*<String,Image>*/ imageMap;
private boolean empty;
private int mouthStyle;
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) {
http://git-wip-us.apache.org/repos/asf/groovy/blob/c5f17abb/src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java b/src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java
index 8609bf6..a831df2 100644
--- a/src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java
+++ b/src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java
@@ -412,7 +412,7 @@ public class MetaClassRegistryImpl implements MetaClassRegistry{
* @param includeExtension
* @return the registry
*/
- public static MetaClassRegistry getInstance(int includeExtension) {
+ public static synchronized MetaClassRegistry getInstance(int includeExtension) {
if (includeExtension != DONT_LOAD_DEFAULT) {
if (instanceInclude == null) {
instanceInclude = new MetaClassRegistryImpl();
http://git-wip-us.apache.org/repos/asf/groovy/blob/c5f17abb/src/main/org/codehaus/groovy/tools/shell/util/Logger.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/tools/shell/util/Logger.java b/src/main/org/codehaus/groovy/tools/shell/util/Logger.java
index 0c457fb..df9ef9f 100644
--- a/src/main/org/codehaus/groovy/tools/shell/util/Logger.java
+++ b/src/main/org/codehaus/groovy/tools/shell/util/Logger.java
@@ -44,9 +44,13 @@ public final class Logger {
private void log(final String level, Object msg, Throwable cause) {
assert level != null;
assert msg != null;
-
+
if (io == null) {
- io = new IO();
+ synchronized (Logger.class) {
+ if (io == null) {
+ io = new IO();
+ }
+ }
}
// Allow the msg to be a Throwable, and handle it properly if no cause is given
http://git-wip-us.apache.org/repos/asf/groovy/blob/c5f17abb/src/main/org/codehaus/groovy/vmplugin/v7/IndyInterface.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/vmplugin/v7/IndyInterface.java b/src/main/org/codehaus/groovy/vmplugin/v7/IndyInterface.java
index e7a04de..d8c7fdf 100644
--- a/src/main/org/codehaus/groovy/vmplugin/v7/IndyInterface.java
+++ b/src/main/org/codehaus/groovy/vmplugin/v7/IndyInterface.java
@@ -116,9 +116,12 @@ public class IndyInterface {
if (LOG_ENABLED) {
LOG.info("invalidating switch point");
}
- SwitchPoint old = switchPoint;
- switchPoint = new SwitchPoint();
- synchronized(IndyInterface.class) { SwitchPoint.invalidateAll(new SwitchPoint[]{old}); }
+
+ synchronized(IndyInterface.class) {
+ SwitchPoint old = switchPoint;
+ switchPoint = new SwitchPoint();
+ SwitchPoint.invalidateAll(new SwitchPoint[]{old});
+ }
}
/**
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>
>
Re: groovy git commit: Fixing squid:S2444 - Lazy initialization of
"static" fields should be "synchronized"
Posted by John Wagenleitner <jo...@gmail.com>.
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>