You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@ant.apache.org by bu...@apache.org on 2007/04/24 16:09:24 UTC

DO NOT REPLY [Bug 42219] New: - Inefficient code in Union, DirectoryScanner makes large copy tasks very slow

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=42219>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=42219

           Summary: Inefficient code in Union, DirectoryScanner makes large
                    copy tasks very slow
           Product: Ant
           Version: 1.7.0
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Core tasks
        AssignedTo: dev@ant.apache.org
        ReportedBy: m101@io.com


In the (apparently) new org.apache.tools.ant.types.resources.Union class, and in
the venerable org.apache.tools.ant.DirectoryScanner class, Vectors or ArrayList
objects are used to accumulate unique file names.  The resultant use of the
"contains" or "indexOf" methods on those objects consumes a tremendous amount of
CPU time when the lists get long. The primary offender is the code in the new
Union class, specifically in the "getCollection" implementation.  The code in
DirectoryScanner is similar but (in my experience) causes less dramatic problems.

I'll attach a test build.xml file.

The solution is simple: since Java 1.4, java.util.LinkedHashSet has been
available to combine the lookup ("contains") performance of a HashSet coupled
with the predictable ordering of ArrayList or Vector.  In the Union class, the
code change necessary is to replace the ArrayList with a LinkedHashSet. In
DirectoryScanner, all the Vector sets can be similarly replaced, though a few
cleanup code changes are necessary (invisible outside the class).

In my real-life build, I have an "install" phase that involves at one point
copying over 12,000 .class files into an application directory. That was taking
about 45 seconds of solid 100% CPU-burning time (without actually doing any
copying; it was a "null build" situation where a build had just been completed),
but after hacking in my own fix to Union.java the build runs in about 15 seconds
(directly comparable to 1.6.5).  By applying the fixes to DirectoryScanner as
well, the "null build" time drops to 10 or 11 seconds.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

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

Re: DO NOT REPLY [Bug 42219] - Inefficient code in Union, DirectoryScanner makes large copy tasks very slow

Posted by Alexey Solofnenko <tr...@gmail.com>.

I would use inheritance instead of facades.

- Alexey.

On 4/26/07, Steve Loughran <st...@apache.org> wrote:
>
> Alexey N. Solofnenko wrote:
> > Classpath ordering is a usual practice that is used, for example, for
> > patching.
>
> yes, and it doesnt work with signed JARs. And, because <fileset> doesnt
> impose an order, you can't guarantee the order of use.
>
>
>   The same classpath order could be used in debugger too. In our
> > case it could be hidden inside launcher. But there are other ways to
> > achieve the same - for example, factories that can return Java6 specific
> > FileResource, but it is cumbersome:
> > (FileResource)project.createObject("org.apache....FileResource"). The
> > later has its advantages too - project could configure the class to
> > support permissions or not.
>
> Facades, all you need are facades. More indirection.
>
>
> >
> > This follows to settings. I think we could put the settings in the
> > project class (as get/set methods or somehow else) - support or not
> > permissions, old/new behaviour is not a property of the environment, but
> > it is a property of a specific build script.
>
> hmmm.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
>


-- 
Alexey N. Solofnenko
Home: http://trelony.cjb.net/
Pleasant Hill, CA (GMT-8 usually)

Re: DO NOT REPLY [Bug 42219] - Inefficient code in Union, DirectoryScanner makes large copy tasks very slow

Posted by Steve Loughran <st...@apache.org>.

Alexey N. Solofnenko wrote:
> Classpath ordering is a usual practice that is used, for example, for 
> patching.

yes, and it doesnt work with signed JARs. And, because <fileset> doesnt 
impose an order, you can't guarantee the order of use.


  The same classpath order could be used in debugger too. In our
> case it could be hidden inside launcher. But there are other ways to 
> achieve the same - for example, factories that can return Java6 specific 
> FileResource, but it is cumbersome: 
> (FileResource)project.createObject("org.apache....FileResource"). The 
> later has its advantages too - project could configure the class to 
> support permissions or not.

Facades, all you need are facades. More indirection.


> 
> This follows to settings. I think we could put the settings in the 
> project class (as get/set methods or somehow else) - support or not 
> permissions, old/new behaviour is not a property of the environment, but 
> it is a property of a specific build script.

hmmm.

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

Re: DO NOT REPLY [Bug 42219] - Inefficient code in Union, DirectoryScanner makes large copy tasks very slow

Posted by "Alexey N. Solofnenko" <A....@mdl.com>.

Classpath ordering is a usual practice that is used, for example, for 
patching. The same classpath order could be used in debugger too. In our 
case it could be hidden inside launcher. But there are other ways to 
achieve the same - for example, factories that can return Java6 specific 
FileResource, but it is cumbersome: 
(FileResource)project.createObject("org.apache....FileResource"). The 
later has its advantages too - project could configure the class to 
support permissions or not.

This follows to settings. I think we could put the settings in the 
project class (as get/set methods or somehow else) - support or not 
permissions, old/new behaviour is not a property of the environment, but 
it is a property of a specific build script.

- Alexey.

Steve Loughran wrote:
> Alexey N. Solofnenko wrote:
>
> Relying on order of stuff in classloaders is just wrong.
> -gets complicated when you bring signing into the bix
> -gets complex when the IDE sets up the build order
> -makes debugging a PITA.
>
>
>> I think we should not limit supported features supported by modern 
>> Java while keeping backward compatibility with older versions. One 
>> way is to use reflection. Another is to provide an alternative 
>> implementation of some classes (FileResource) to be used with modern 
>> Java. The later is straightforward, but the most recent Java is 
>> required to build it.
>
> That's effectively what we do at build time...you need to build under 
> 1.5 to get everything compiled, though we omit the stuff to let people 
> still build on java 1.3+.
>
> For the proxy diagnostics, I abuse the toString() method, so we just 
> cast the proxy diags to a method and do it from there. For the other 
> stuff, well, there's nothing to stop FileUtils looking for a subclass 
> of itself if it is there
>
>>
>> If something could be done better with modern Java, why not to use it 
>> when possible?
>
> Only one reason: when it makes backwards compatibility odd, or screws 
> up x-platformness. For example, for all the failings with file perms 
> on java, requiring people to spec permissions when tarring a folder 
> guarantees that the permissions in the tar get set in all platforms
>
> At the same time, sometimes I do want copy to preserve stuff, untar to 
> propagate permissions, at least on unix. I may even want to use file 
> permissions as a selector, selecting all files that are executable for 
> some further action. As long as people understand the operations are 
> not portable, well, they have the right to use them.
>
> Perhaps a limited set of operations could have permissions enabled, 
> with some enum like
> permissions="always"   - copy all perms, fail if we are a windows box 
> or if the build doesnt allow it (wrong JVM, wrong class file)
> permissions="never"     -the default
> permissions="optional"  -copy perms if the JVM is right and the OS 
> supports perms.
>
> we could also add another Permissions interface to resources, so you 
> can read/write perms of files. This would work inside zip/tar files 
> today, and on filesystem files on Java6+unix.
>
> I worry about what cygwin does here...how does it manage permissions? 
> Because people are going to be disappointed if they expect ant to 
> handle it.
> -Steve
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org

-- 
------------------------------------------------------------------------
Alexey N. Solofnenko <http://trelony.cjb.net/>
Pleasant Hill, CA (GMT-8 usually)

Re: DO NOT REPLY [Bug 42219] - Inefficient code in Union, DirectoryScanner makes large copy tasks very slow

Posted by Steve Loughran <st...@apache.org>.

Alexey N. Solofnenko wrote:

Relying on order of stuff in classloaders is just wrong.
-gets complicated when you bring signing into the bix
-gets complex when the IDE sets up the build order
-makes debugging a PITA.

> I think we should not limit supported features supported by modern Java 
> while keeping backward compatibility with older versions. One way is to 
> use reflection. Another is to provide an alternative implementation of 
> some classes (FileResource) to be used with modern Java. The later is 
> straightforward, but the most recent Java is required to build it.

That's effectively what we do at build time...you need to build under 
1.5 to get everything compiled, though we omit the stuff to let people 
still build on java 1.3+.

For the proxy diagnostics, I abuse the toString() method, so we just 
cast the proxy diags to a method and do it from there. For the other 
stuff, well, there's nothing to stop FileUtils looking for a subclass of 
itself if it is there

> 
> If something could be done better with modern Java, why not to use it 
> when possible?

Only one reason: when it makes backwards compatibility odd, or screws up 
x-platformness. For example, for all the failings with file perms on 
java, requiring people to spec permissions when tarring a folder 
guarantees that the permissions in the tar get set in all platforms

At the same time, sometimes I do want copy to preserve stuff, untar to 
propagate permissions, at least on unix. I may even want to use file 
permissions as a selector, selecting all files that are executable for 
some further action. As long as people understand the operations are not 
portable, well, they have the right to use them.

Perhaps a limited set of operations could have permissions enabled, with 
some enum like
permissions="always"   - copy all perms, fail if we are a windows box or 
if the build doesnt allow it (wrong JVM, wrong class file)
permissions="never" 	-the default
permissions="optional"  -copy perms if the JVM is right and the OS 
supports perms.

we could also add another Permissions interface to resources, so you can 
read/write perms of files. This would work inside zip/tar files today, 
and on filesystem files on Java6+unix.

I worry about what cygwin does here...how does it manage permissions? 
Because people are going to be disappointed if they expect ant to handle it.
-Steve

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

Re: DO NOT REPLY [Bug 42219] - Inefficient code in Union, DirectoryScanner makes large copy tasks very slow

Posted by "Alexey N. Solofnenko" <A....@mdl.com>.

I think we should not limit supported features supported by modern Java 
while keeping backward compatibility with older versions. One way is to 
use reflection. Another is to provide an alternative implementation of 
some classes (FileResource) to be used with modern Java. The later is 
straightforward, but the most recent Java is required to build it.

If something could be done better with modern Java, why not to use it 
when possible?

- Alexey.

Peter Reilly wrote:
> For file permissions, I think we have to use refection to see if
> the file object supports the api.  I do not think that we can
> use pure java to check the permissions (except by using
> exec hackery) for pre java 1.4.
>
> Peter
>
> On 4/24/07, Alexey Solofnenko <tr...@gmail.com> wrote:
>> "No" for file permissions too?
>>
>> - Alexey.
>>
>> On 4/24/07, bugzilla@apache.org <bu...@apache.org> wrote:
>> >
>> > DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
>> > RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
>> > <http://issues.apache.org/bugzilla/show_bug.cgi?id=42219>.
>> > ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
>> > INSERTED IN THE BUG DATABASE.
>> >
>> > http://issues.apache.org/bugzilla/show_bug.cgi?id=42219
>> >
>> >
>> >
>> >
>> >
>> > ------- Additional Comments From peterreilly@apache.org  2007-04-24 
>> 10:09
>> > -------
>> > #1
>> > Thanks mike for the build.xml, it would be nicer
>> > to be completel self contained - perhaps a
>> > bash/python script to create a big directory, and the
>> > build.xml to use this (see:
>> >
>> > 
>> http://svn.apache.org/viewvc/ant/core/trunk/src/etc/performance/build.xml?revision=448449&view=markup 
>>
>> > for an example of a selcontained test.
>> >
>> > #5
>> > I do not think that we should do this (provide a pre-jdk1.4 jar) for
>> > this problem.
>> >
>> >
>> >
>> > --
>> > Configure bugmail:
>> > http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
>> > ------- You are receiving this mail because: -------
>> > You are the assignee for the bug, or are watching the assignee.
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
>> > For additional commands, e-mail: dev-help@ant.apache.org
>> >
>> >
>>
>>
>> -- 
>> Alexey N. Solofnenko
>> Home: http://trelony.cjb.net/
>> Pleasant Hill, CA (GMT-8 usually)
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org

-- 
------------------------------------------------------------------------
Alexey N. Solofnenko <http://trelony.cjb.net/>
Pleasant Hill, CA (GMT-8 usually)

Re: DO NOT REPLY [Bug 42219] - Inefficient code in Union, DirectoryScanner makes large copy tasks very slow

Posted by Peter Reilly <pe...@gmail.com>.

For file permissions, I think we have to use refection to see if
the file object supports the api.  I do not think that we can
use pure java to check the permissions (except by using
exec hackery) for pre java 1.4.

Peter

On 4/24/07, Alexey Solofnenko <tr...@gmail.com> wrote:
> "No" for file permissions too?
>
> - Alexey.
>
> On 4/24/07, bugzilla@apache.org <bu...@apache.org> wrote:
> >
> > DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
> > RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
> > <http://issues.apache.org/bugzilla/show_bug.cgi?id=42219>.
> > ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
> > INSERTED IN THE BUG DATABASE.
> >
> > http://issues.apache.org/bugzilla/show_bug.cgi?id=42219
> >
> >
> >
> >
> >
> > ------- Additional Comments From peterreilly@apache.org  2007-04-24 10:09
> > -------
> > #1
> > Thanks mike for the build.xml, it would be nicer
> > to be completel self contained - perhaps a
> > bash/python script to create a big directory, and the
> > build.xml to use this (see:
> >
> > http://svn.apache.org/viewvc/ant/core/trunk/src/etc/performance/build.xml?revision=448449&view=markup
> > for an example of a selcontained test.
> >
> > #5
> > I do not think that we should do this (provide a pre-jdk1.4 jar) for
> > this problem.
> >
> >
> >
> > --
> > Configure bugmail:
> > http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
> > ------- You are receiving this mail because: -------
> > You are the assignee for the bug, or are watching the assignee.
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> > For additional commands, e-mail: dev-help@ant.apache.org
> >
> >
>
>
> --
> Alexey N. Solofnenko
> Home: http://trelony.cjb.net/
> Pleasant Hill, CA (GMT-8 usually)
>

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