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 <[email protected]> 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
>> <[email protected] <mailto:[email protected]>> 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, <[email protected]
>>> <mailto:[email protected]>> 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>
>>>
>>>
>>
>>
