tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: svn commit: r1703290 - /tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java
Date Wed, 16 Sep 2015 19:24:31 GMT
Mark,

On 9/16/15 2:11 PM, Mark Thomas wrote:
> On 16/09/2015 14:29, Christopher Schultz wrote:
>> Mark,
>>
>> On 9/15/15 5:10 PM, markt@apache.org wrote:
>>> Author: markt
>>> Date: Tue Sep 15 21:10:30 2015
>>> New Revision: 1703290
>>>
>>> URL: http://svn.apache.org/r1703290
>>> Log:
>>> Follow-up to r1703177.
>>> Ensure that members never contains an intermediate result of the sorting process.
>>>
>>> Modified:
>>>     tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java
>>>
>>> Modified: tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java
>>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java?rev=1703290&r1=1703289&r2=1703290&view=diff
>>> ==============================================================================
>>> --- tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java (original)
>>> +++ tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java Tue
Sep 15 21:10:30 2015
>>> @@ -66,7 +66,7 @@ public class Membership implements Clone
>>>              final HashMap<Member, MbrEntry> tmpclone = (HashMap<Member,
MbrEntry>) map.clone();
>>>              clone.map = tmpclone;
>>>              clone.members = new Member[members.length];
>>> -            System.arraycopy(members,0,clone.members,0,members.length);
>>> +            System.arraycopy(members, 0, clone.members, 0, members.length);
>>>              return clone;
>>>          }
>>>      }
>>> @@ -134,7 +134,12 @@ public class Membership implements Clone
>>>                      updateMember.setMemberAliveTime(member.getMemberAliveTime());
>>>                      updateMember.setPayload(member.getPayload());
>>>                      updateMember.setCommand(member.getCommand());
>>> -                    Arrays.sort(members, memberComparator);
>>> +                    // Re-order. Can't sort in place since a call to
>>> +                    // getMembers() may then receive an intermediate result.
>>> +                    Member[] newMembers = new Member[members.length];
>>> +                    System.arraycopy(members, 0, newMembers, 0, members.length);
>>> +                    Arrays.sort(newMembers, memberComparator);
>>> +                    members = newMembers;
>>
>> Consider using Member[].clone here instead of creating your own array
>> and manually calling System.arraycopy. Less code and fewer opportunities
>> to mass things up. Same performance.
> 
> I'll get this done shortly but I did want to say you should feel free to
> make this sort of change directly. I'm happy to do it as a review
> comment on one of my patches but I'm not going complain if you want to
> go ahead and do this yourself.

Okay. I mostly didn't want to step on your toes if you were doing more
refactoring. For the tribes classes, you commented recently that there
looked to be a great many improvements to be made in there... I wasn't
sure if you had some kind of refactoring in-progress.

As for System.arraycopy() -> array[].clone, I also wasn't sure if you
were doing it for a reason that wasn't obvious from the diff posted
here, or if you had a moral objection to that kind of change :)

Oddly enough, array[].clone() is JVM magic: I can't discover it's actual
implementation... a debugger steps right over the call just like it
would for an assignment. As far as Eclipse is concerned, array[].clone
is *not* a method call.

-chris


Mime
View raw message