Bug 69142 - FileResourceSet allocates unnecessary Strings
Summary: FileResourceSet allocates unnecessary Strings
Status: RESOLVED WONTFIX
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.x
Hardware: All All
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-06-19 13:48 UTC by John Engebretson
Modified: 2024-06-20 16:42 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John Engebretson 2024-06-19 13:48:50 UTC
The method 'org.apache.catalina.webresources.FileResourceSet.getResource(String)' has logic that in the common case (for us, anyway) appends a '/' to a String, then immediately strips it back out.  The repeated concatenation means repeated object allocation, and as this is a hot path, it impacts our startup time and our request-handling time (we have unfortunate classpath scans during each request).

In version 9.x, the append occurs at FileResourceSet.java#L83 and the character is stripped at FileResourceSet.java#L87.  Rewriting that code to avoid the duplication would be isolated, safe, and helpful.

A clunky example of that rewrite is:

if (path.charAt(path.length() - 1) != '/')
    if (webAppMount.startsWith(path)) {
        String name = path;
        name = name.substring(name.lastIndexOf('/') + 1);
        if (name.length() > 0) {
            return new VirtualResource(root, path, name);
        }
    } else {
        path = path + '/';
    }
else if (webAppMount.startsWith(path)) {
    String name = path.substring(0, path.length() - 1);
    name = name.substring(name.lastIndexOf('/') + 1);
    if (name.length() > 0) {
        return new VirtualResource(root, path, name);
    }
}
return new EmptyResource(root, path);
Comment 1 Christopher Schultz 2024-06-19 17:02:50 UTC
The code is not equivalent. For reference, the original code:

Line
82        if (path.charAt(path.length() - 1) != '/') {
83            path = path + '/';
84        }
85
86        if (webAppMount.startsWith(path)) {
87            String name = path.substring(0, path.length() - 1);
88            name = name.substring(name.lastIndexOf('/') + 1);
89            if (name.length() > 0) {
90                return new VirtualResource(root, path, name);
91            }
92        }

The proposed solution avoids appending "/" on line 83 but then the predicate on line 86 is not modified in the proposal, and webappMount.startsWith(path) is no longer guaranteed to end in a "/".

I think the proposal needs a little adjustment.
Comment 2 John Engebretson 2024-06-19 19:56:59 UTC
Good catch!  I wrote a fix, unit test, and speed test, and discovered that the corrected logic performs comparably to the old.  In short, my proposed fix is more complicated than current but performance neutral.

Please close as wont fix or such.  My apologies.  :)
Comment 3 Mark Thomas 2024-06-20 10:47:44 UTC
Closing as requested. Happy to look at this again if you come up with a proposal that improves performance.
Comment 4 Christopher Schultz 2024-06-20 16:42:39 UTC
We could keep track of both the "appended" string as well as the trimmed string. That is, in the case where the path does not end in a "/" we could keep the "naked" path and re-use it later instead of adding, then trimming.

The rest of the logic could remain the same.

For example:

if (path.charAt(path.length() - 1) != '/')
    String withTrailingSlash = path + "/";

    if (webAppMount.startsWith(withTrailingSlash)) {
        String name = path; // Without trailing slash
        name = name.substring(name.lastIndexOf('/') + 1);
        if (name.length() > 0) {
            return new VirtualResource(root, path, name);
        }
    } else {
        path = withTrailingSlash;
    }
else if (webAppMount.startsWith(path)) {
    String name = path.substring(0, path.length() - 1);
    name = name.substring(name.lastIndexOf('/') + 1);
    if (name.length() > 0) {
        return new VirtualResource(root, path, name);
    }
}