ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jose Alberto Fernandez" <jalbe...@cellectivity.com>
Subject RE: AW: [Patch] modifiedselector, style, remove unused code, slightly more lazy DigestAlgorithm.getValue (now with added source code -doh!)
Date Mon, 28 Feb 2005 10:41:35 GMT
Just one comment on your ClosureUtils.map() method.

Although this code may be very good for ArrayList, I think
it will behave quite bad for LinkList arguments where
the cost of List.get(i) is quite expensive.

Maybe you should provide for that if you want this as a general
utility pattern.

Jose Alberto

> -----Original Message-----
> From: Kev Jackson [mailto:kevin.jackson@it.fts-vn.com] 
> Sent: 28 February 2005 07:54
> To: Ant Developers List
> Subject: Re: AW: [Patch] modifiedselector, style, remove 
> unused code, slightly more lazy DigestAlgorithm.getValue (now 
> with added source code -doh!)
> 
> 
> 
> >Why you use the for-loop instead of iterator?
> >  
> >
> 
> 1 iterator has to check underlying array bounds for every 
> hasNext(), and 
> every next(), checking is slow, and in this case unecessary
> 2 iterator call to hasNext() each time around the loop - again we can 
> call size() once and cache the result, since we are never 
> updating the 
> underlying structure, this won't change and the local var 
> read is faster 
> than a method call.
> 3 iterator is a "large" object to create, an int is a small one 
> (bytecode is probably smaller, although I haven't checked this, I can 
> almost (99%) guarantee it)
> 4 remove calls to iterator, then we can remove the import :)
> 
> It's just an idiom I learnt from "Effective Java" or one of the books 
> like that, but the reasons above mean that I use it whenever 
> I can, as 
> it does tend to speed up large loops, at the expense of only a very 
> slight obfuscation of the code.  Some people use essentially the same 
> loop but move the size variable above the loop.  That's ok, 
> but it means 
> that the size variable has slightly larger scope than it requires, 
> although the effect is the same.
> 
> Also speaking of Iterators in general...
> 
> Iterators: Signs of Weakness in Object-Oriented Languages 
> http://citeseer.ist.psu.edu/55669.html (there's a PDF or PS file 
> downloadable from the link at the top)
> 
> I don't pretend to understand all the Lispness of this paper, 
> but some 
> of the arguments against external iterators (the kind we have 
> in Java) 
> are interesting.  I did a little test last week comparing a  normal 
> iterator/for loop, with a "functional-style" of programming, both in 
> Java.  Speed was identical, but bytecode of the functional-style was 
> much smaller, there were less casts and less method calls.  
> Anecdotal at 
> best, but try it yourself:
> 
> [I decided to call these things Closures, but they aren't 
> quite, rather 
> more like Blocks, correct me if I'm getting my terms wrong]
> 
> public interface Closure {
>     Object execute(Object o);
> }
> 
> import java.util.ArrayList;
> import java.util.List;
> 
> public class ClosureUtils {
> 
>     /**
>      * This method maps a "function" to a list of data
>      * and returns a new modified list.
>      * @param list the source list of data
>      * @param c the Closure to apply to the list
>      * @return the modified list
>      */
>     public static List map(final List list, final Closure c) {
>         final int size = list.size();
>         List result = new ArrayList(size);
>         for (int i=0; i<size; i++) {
>             result.add(c.execute(list.get(i)));
>         }
>         return result;
>     }
> }
> 
> import java.util.ArrayList;
> import java.util.Arrays;
> import java.util.Date;
> import java.util.Iterator;
> import java.util.List;
> 
> import junit.framework.TestCase;
> 
> 
> public class ClosureUtilsTest extends TestCase {
>    
>     List testList = Arrays.asList(new String[]{"This", 
> "That", "Other"});
>    
>     /*
>      * @see TestCase#setUp()
>      */
>     protected void setUp() throws Exception {
>         super.setUp();
>     }
>    
>     /*
>      * @see TestCase#tearDown()
>      */
>     protected void tearDown() throws Exception {
>         super.tearDown();
>     }
>    
>     public void testJ5() {
>         try {
>             Date start = new Date();
>             System.out.println("Start J5");
>             for (int i=0; i<100000; i++) {
>                 List newList = new ArrayList();
>                
>                 for (Object o : testList) {
>                     newList.add(((String)o).toUpperCase());
>                 }
>                
>                 assertEquals(((String)newList.get(0)), 
> ((String)testList.get(0)).toUpperCase());
>             }
>             System.out.println("End elapsed : [ "+(new 
> Date().getTime()-start.getTime()));
>         } catch (Exception e) {
>             e.printStackTrace();
>             fail();
>         }
>     }
>    
>     public void testOld() {
>         try {
>             Date start = new Date();
>             System.out.println("Start old");
>             for (int i=0; i<100000; i++) {
>                 List newList = new ArrayList();
>                
>                 for (Iterator it = testList.iterator(); 
> it.hasNext();) {
>                     newList.add(((String)it.next()).toUpperCase());
>                 }
>                
>                 assertEquals(((String)newList.get(0)), 
> ((String)testList.get(0)).toUpperCase());
>             }
>             System.out.println("End elapsed : [ "+(new 
> Date().getTime()-start.getTime()));
>         } catch (Exception e) {
>             e.printStackTrace();
>             fail();
>         }
>     }
>    
>     public void testMap() {
>         try {
>             Date start = new Date();
>             System.out.println("Start closure");
>             for (int i=0; i<100000; i++) {
>                 List newList = ClosureUtils.map(testList, new 
> Closure() {
>                     public Object execute(Object o) {
>                         return ((String)o).toUpperCase();
>                     }
>                 }
>                 );
>                 assertEquals(((String)newList.get(0)), 
> ((String)testList.get(0)).toUpperCase());
>             }
>             System.out.println("End elapsed : [ "+(new 
> Date().getTime()-start.getTime()));
>         } catch (Exception e) {
>             e.printStackTrace();
>             fail();
>         }
>     }
> }
> 
> bytecode (note this is from tests without the 100000 
> iterations, or the 
> timing code, both compiled on jdk1.5.0-b64/winXP, output from javap):
> 
> testJ5/testOld produced identical bytecode, showing that the new 
> sugar-coated for loop is simply an old loop underneath
> 
> public void testMap();
> 
>   Code:
>    0:   aload_0
>    1:   getfield        #27; //Field testList:Ljava/util/List;
>    4:   new     #43; //class com/ftsvn/tools/test/ClosureUtilsTest$1
>    7:   dup
>    8:   aload_0
>    9:   invokespecial   #46; //Method 
> com/ftsvn/tools/test/ClosureUtilsTest$1."<
> init>":(Lcom/ftsvn/tools/test/ClosureUtilsTest;)V
>    12:  invokestatic    #52; //Method 
> com/ftsvn/tools/ClosureUtils.map:(Ljava/ut
> il/List;Lcom/ftsvn/tools/Closure;)Ljava/util/List;
>    15:  astore_1
>    16:  aload_1
>    17:  iconst_0
>    18:  invokeinterface #58,  2; //InterfaceMethod 
> java/util/List.get:(I)Ljava/l
> ang/Object;
>    23:  checkcast       #13; //class java/lang/String
>    26:  checkcast       #13; //class java/lang/String
>    29:  aload_0
>    30:  getfield        #27; //Field testList:Ljava/util/List;
>    33:  iconst_0
>    34:  invokeinterface #58,  2; //InterfaceMethod 
> java/util/List.get:(I)Ljava/l
> ang/Object;
>    39:  checkcast       #13; //class java/lang/String
>    42:  checkcast       #13; //class java/lang/String
>    45:  invokevirtual   #62; //Method 
> java/lang/String.toUpperCase:()Ljava/lang/
> String;
>    48:  invokestatic    #68; //Method 
> junit/framework/Assert.assertEquals:(Ljava
> /lang/String;Ljava/lang/String;)V
>    51:  goto    62
>    54:  astore_1
>    55:  aload_1
>    56:  invokevirtual   #71; //Method 
> java/lang/Exception.printStackTrace:()V
>    59:  invokestatic    #74; //Method junit/framework/Assert.fail:()V
>    62:  return
>   Exception table:
>    from   to  target type
>      0    54    54   Class java/lang/Exception
> 
> public void testOld();
>   Code:
>    0:   new     #80; //class java/util/ArrayList
>    3:   dup
>    4:   invokespecial   #81; //Method java/util/ArrayList."<init>":()V
>    7:   astore_1
>    8:   aload_0
>    9:   getfield        #27; //Field testList:Ljava/util/List;
>    12:  invokeinterface #85,  1; //InterfaceMethod 
> java/util/List.iterator:()Lja
> va/util/Iterator;
>    17:  astore_2
>    18:  goto    43
>    21:  aload_1
>    22:  aload_2
>    23:  invokeinterface #91,  1; //InterfaceMethod 
> java/util/Iterator.next:()Lja
> va/lang/Object;
>    28:  checkcast       #13; //class java/lang/String
>    31:  checkcast       #13; //class java/lang/String
>    34:  invokevirtual   #62; //Method 
> java/lang/String.toUpperCase:()Ljava/lang/
> String;
>    37:  invokeinterface #95,  2; //InterfaceMethod 
> java/util/List.add:(Ljava/lan
> g/Object;)Z
>    42:  pop
>    43:  aload_2
>    44:  invokeinterface #99,  1; //InterfaceMethod 
> java/util/Iterator.hasNext:()
> Z
>    49:  ifne    21
>    52:  aload_1
>    53:  iconst_0
>    54:  invokeinterface #58,  2; //InterfaceMethod 
> java/util/List.get:(I)Ljava/l
> ang/Object;
>    59:  checkcast       #13; //class java/lang/String
>    62:  checkcast       #13; //class java/lang/String
>    65:  aload_0
>    66:  getfield        #27; //Field testList:Ljava/util/List;
>    69:  iconst_0
>    70:  invokeinterface #58,  2; //InterfaceMethod 
> java/util/List.get:(I)Ljava/l
> ang/Object;
>    75:  checkcast       #13; //class java/lang/String
>    78:  checkcast       #13; //class java/lang/String
>    81:  invokevirtual   #62; //Method 
> java/lang/String.toUpperCase:()Ljava/lang/
> String;
>    84:  invokestatic    #68; //Method 
> junit/framework/Assert.assertEquals:(Ljava
> /lang/String;Ljava/lang/String;)V
>    87:  goto    98
>    90:  astore_1
>    91:  aload_1
>    92:  invokevirtual   #71; //Method 
> java/lang/Exception.printStackTrace:()V
>    95:  invokestatic    #74; //Method junit/framework/Assert.fail:()V
>    98:  return
>   Exception table:
>    from   to  target type
>      0    90    90   Class java/lang/Exception
> 
>  From this I was fairly impressed on the code-size reduction 
> alone, but 
> the fact that you don't trade-off speed is excellent.  It behaves 
> similarly to the Visitor pattern I think, but instead of an 
> accept/visit 
> method you have an execute.  I like the style, but as I mentioned 
> earlier I'm not convinced I should name them Closures.  
> Anyway I'm rambling.
> 
> >>      //
> >>       // -----  Instantiate the interfaces  -----
> >>       //
> >>       String className = null;
> >>       String pkg =
> >>"org.apache.tools.ant.types.selectors.modifiedselector";
> >>
> >>what do these lines do in ModifiedSelector.configure?
> >>Eclipse says that 
> >>they're never read, and as they're method variables, not class or 
> >>instance variables (ie not public), I was very tempted to 
> >>delete them, 
> >>but I thought I'd better ask first in case they're present 
> to support 
> >>future functionality
> >>    
> >>
> >
> >That came more from "past functionality" than for future one 
> :) While 
> >developing the modified selector I started with reflection. And in 
> >earlier steps I had hardcoded classnames for the three interfaces as 
> >default. And so I could simply shorten
> >the text to write.
> >My ascii editor I used there couldnĀ“t check for unneeded variables :)
> >
> >  
> >
> Ah, ok, I've removed them, here's the "new" modified selector
> 
> Kev
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


Mime
View raw message