I would have concerns about moving the match code and especially about changing includes/excludes in DirectoryScanner. A lot of various things in Ant reference it (as I discovered when I tried to move it ;). Couldn't you just apply your patch to the existing methods?
As a side note, I think a major performance improvement would come from caching the pattern arrays. Right now DS parses the pattern strings every time a match call is made. d On Mon, 26 Mar 2001 21:46:14 -0500, Eric Siegerman wrote: >For our project, we want to explicitly specify which java files >are compiled, rather than trust ant's usual catchall >"src/**/*.java" approach. So I made a manifest file listing all >the desired files (250 or so), and referred to it in: > <javac includesfile=.../> > >... only to discover that ant took agonizingly long to figure out >which files to recompile. This turned out to be because of the >expensive matchPath() calls on all of those 250 non-patterns. > >Taken together, the two attached patches solve this problem, by >doing a simple string-compare when the pattern is a simple >string, ie. when it contains no pattern metacharacters. > >I've split this into two patches for readability. Patch 1 is a >refactoring; it extracts the pattern-matching code out of >DirectoryScanner into a new Pattern class. Patch 2 adds the >simple-string-compare optimization to Pattern. There should be >no user-visible change (except speed, of course). > >The savings are dramatic. Here are some timings (taken with no >compilations needed, so that most of the work was ant overhead). >The "ant 1.3" test was a default build, ie. including the "jars" >and "dist-lite" targets. The "our project" test includes a ><depend> task that takes several seconds. > > Build > ant our >Use 1.3 project >--- --- ------- >Vanilla ant 1.3 25" 71" >Patched ant 1.3 15" 8" >Savings 40% 89% > >Even on ant's build.xml itself, which doesn't use simple-filename >"patterns" nearly as heavily as our project, the optimization >saves 40%. > >I've included the two patches (against the Release 1.3 sources), >and a new PatternTest.java with a bezillion test cases. > >PROBLEMS: The code seems to run fine, but the JUnit tests have >two issues: > > 1. The PatternTest class runs fine from the command line (except > for a few failed tests; see below), but if I run it using > "ant run-tests", *every* test case throws the following > exception: > > Testcase: test000 took 0.134 sec > Caused an ERROR > try to access class org/apache/tools/ant/Pattern from class > org/apache/tools/ant/PatternTest > java.lang.IllegalAccessError: try to access class > org/apache/tools/ant/Pattern from class org/apache/tools/ant/PatternTe > > at org.apache.tools.ant.PatternTest.tstPath(PatternTest.java:92) > at org.apache.tools.ant.PatternTest.test000(PatternTest.java:114) > at java.lang.reflect.Method.invoke(Native Method) > at junit.framework.TestCase.runTest(TestCase.java:156) > at junit.framework.TestCase.runBare(TestCase.java:130) > at junit.framework.TestResult$1.protect(TestResult.java:106) > at junit.framework.TestResult.runProtected(TestResult.java:124) > at junit.framework.TestResult.run(TestResult.java:109) > at junit.framework.TestCase.run(TestCase.java:121) > at junit.framework.TestSuite.runTest(TestSuite.java:157) > at junit.framework.TestSuite.run(TestSuite.java:152) > at > org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:209) > at > org.apache.tools.ant.taskdefs.optional.junit.JUnitTask.executeInVM(JUnitTask.java:409) > at > org.apache.tools.ant.taskdefs.optional.junit.JUnitTask.execute(JUnitTask.java:283) > at > org.apache.tools.ant.taskdefs.optional.junit.JUnitTask.execute(JUnitTask.java:263) > at org.apache.tools.ant.Target.execute(Target.java:153) > at org.apache.tools.ant.Project.runTarget(Project.java:898) > at org.apache.tools.ant.Project.executeTarget(Project.java:536) > at org.apache.tools.ant.Project.executeTargets(Project.java:510) > at org.apache.tools.ant.Main.runBuild(Main.java:421) > at org.apache.tools.ant.Main.main(Main.java:149) > > I haven't a clue why this could be. It may well be a simple > error -- I've never used JUnit before. (I saw a similar > report in another context on the ant-dev archive, with a > suggested fix of instantiating AntClassLoader objects with a > systemFirst argument of "false" -- but <junit> already does > that!) > > Help! > > > 2. Several tests fail. The obvious approach of checking how > they worked before my change is problematic because: > - The way I wrote the test cases, I can't do this -- they > depend on the Pattern class's having been factored out. > - It wouldn't be helpful anyway. Quite possibly, the bugs > (if any) were already there in the preexisting code. > > I need some feedback from people who've been working with ant > longer than I have (which wouldn't be hard; I've only been > here a week). My question is, what are the *expected* > results from the tests in question? Before I go "fixing" the > code, I want to make sure it's broken... > > Here are the cases, and what I *think* the expected results > should be. The "method" column shows the method being tested > (in ant 1.3, these are in DirectoryScanner, but my Patch 1 > moves them to a new Pattern class). > > Candidate Expected > test # Method Pattern Filename Result > ====== ================= ========= ========== ======== > # > # Empty(ish)-filename tests > # > 003 matchPath "/*" "/" true > 004 matchPath "*" "" true > 2000 matchPatternStart "/aa/bb" "/" true > 2001 matchPatternStart "aa/bb" "" true > > > # > # Filename ending in "/". In all of these cases, the > # trailing "/" has no effect; my expected "false" assumes it > # should have one. Should it? > # > 1041 matchPath "/aa/*/cc" "/aa/bb/cc/" false > 2041 matchPatternStart "/aa/*/cc" "/aa/bb/cc/" false > 2043 matchPatternStart "/aa/*/cc" "/aa/" false > 2063 matchPatternStart "/aa/?/cc" "/aa/b/cc/" false > > # > # Pattern ending in "/". I think I recall reading that "/aa/" is > # supposed to be equivalent to "/aa/**"; is this correct? > # > 1160 matchPath "/aa/" "/aa/cc" true > 2160 matchPatternStart "/aa/" "/aa/cc" true > >Thanks much!
