Bug 56801 - Avoid duplicated String to CharArray conversion in the loop of Matcher#matchName
Avoid duplicated String to CharArray conversion in the loop of Matcher#matchName
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
trunk
All All
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2014-08-01 02:31 UTC by Sheldon Shao
Modified: 2014-08-12 13:54 UTC (History)
0 users



Attachments
Patch for Matcher (2.82 KB, patch)
2014-08-01 02:31 UTC, Sheldon Shao
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sheldon Shao 2014-08-01 02:31:24 UTC
Created attachment 31865 [details]
Patch for Matcher

The loop in matchName,

    public static boolean matchName(Set<String> patternSet, String fileName) {
        for (String pattern: patternSet) {
            if (match(pattern, fileName, true)) {
                return true;
            }
        }
        return false;
    }


Optimized,

    public static boolean matchName(Set<String> patternSet, String fileName) {
        char[] charArray = fileName.toCharArray();
        for (String pattern: patternSet) {
            if (match(pattern, charArray, true)) {
                return true;
            }
        }
        return false;
    }
Comment 1 Christopher Schultz 2014-08-01 22:13:31 UTC
I think the real question is whether or not the String.toCharArray even belongs here. Back in the dark days, only fools used String.charAt and instead used String.toCharArray followed by direct index access. I haven't done any performance measurements, but String.toCharArray may be more trouble than it's worth these days.
Comment 2 Mark Thomas 2014-08-05 07:39:34 UTC
Based on the quality of patches the OP has provided in the past, I'm confident that a) the proposed change will be faster and b) there is unlikely to be a better way.

I've applied a variation of this patch to 8.0.x for 8.0.11 onwards.
Comment 3 Konstantin Kolinko 2014-08-12 13:54:31 UTC
(In reply to Christopher Schultz from comment #1)
> I think the real question is whether or not the String.toCharArray even
> belongs here.

Agreed. I changed visibility of the new method to be private, so that we were able to review the API later.  There is also a question that patterns are also repeatedly converted to arrays.

I backported this to Tomcat 7 in r1617475 and it will be in 7.0.56 onwards.