Bug 22266 - Keep template text together with mappedfile="true"
Summary: Keep template text together with mappedfile="true"
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Jasper (show other bugs)
Version: Nightly Build
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2003-08-09 01:27 UTC by Eric Carmichael
Modified: 2004-11-16 19:05 UTC (History)
0 users



Attachments
Proposed patch (2.90 KB, patch)
2003-08-09 01:30 UTC, Eric Carmichael
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carmichael 2003-08-09 01:27:36 UTC
Jasper's mappedfile=true option doesn't always generate static content with 
one print statement per input line.  Jasper creates a new Node every time it 
sees a "<" in the input, so when template text contains an HTML tag in the 
middle of an input line, the resulting Java code is split across multiple 
output lines (and in my opinion is therefore harder to read).

This can be fixed by checking the text after the "<" before moving on to the 
next Node.  I will attach a patch that implements this.
Comment 1 Eric Carmichael 2003-08-09 01:30:31 UTC
Created attachment 7704 [details]
Proposed patch
Comment 2 Remy Maucherat 2003-08-09 19:22:29 UTC
It looks that was what was originally intended, so it's likely another great
patch :)
I'll apply both of your patches.
Comment 3 Kin-Man Chung 2003-08-12 17:21:39 UTC
I would have preferred that this optimization be done with a separate pass over
the parsed nodes, which combines contiguous template text nodes into one.  The
look-ahead used in this patch duplicates logic used in the other part of the
Parser (it also misses <jsp:text>, for instance), and may have some performance
impact.

I'll leave this patch in for now, but may undo it when the separate optim pass
is implemented later, unless Eric beats me to it!  :-)
Comment 4 Eric Carmichael 2003-08-13 02:09:27 UTC
Unfortunately I've thrown out the actual numbers I got in the performance 
testing I did (all on a 1 GHz Windows XP laptop), but the salient results were 
as follows:
1) The patch made Tomcat faster at compiling and serving pages with a 
preponderance of HTML tags versus JSP tags (the lookahead does take some extra 
time, but is more than made up if the lookahead results in one less node to be 
traversed later).  With the patch in place, Tomcat compiled and served 
index.html from the jsp-examples webapp (renamed to index.jsp, obviously) 
about 9% faster than without the patch.
2) On balance, however, the patch made Tomcat slower at compiling and serving 
pages.  At least, when I tested against the jsp-examples webapp (with 
index.jsp renamed back to index.html so it wasn't part of the test), time to 
compile and serve was about 2% slower.
3) I couldn't find any difference in serving compiled pages with the patch in 
place (I thought there might be a small improvement because the patch shrinks 
the .class files).

Based on those numbers, I decided not to submit the patch as an option 
configurable separately from mappedfile, or as a non-configurable 
enhancement.  But since the mappedfile documentation said it was intended for 
debugging, I thought the performance hit was acceptable.

I didn't test your idea of a separate pass through the parsed nodes, but I did 
test code that, when about to create a new TemplateText node, checked to see 
if the previous node was also a TemplateText node, and if so, just appended 
its text to the previous node's (the ideas being that anything that conserved 
nodes might improve performance, and that I didn't like duplicating logic from 
another part of the Parser, either).  But that was noticeably slower (I've 
forgotten the numbers entirely) than the patch I submitted.

Anyway, I submitted the patch as an enhancement, so I can't complain if it 
gets implemented differently or not at all.  The above is just to help folks 
make an informed decision.
Comment 5 Kin-Man Chung 2003-08-13 18:43:14 UTC
There is no doubt that concatenating contiguous template texts into one improves
the runtime performance and I am not too worry about spending extra time during
compilation, since I believe web applications should be deployed precompiled.

My suggestion for a separate pass was motivated by the design principle to keep
the modules simple and direct.  The current compiler is made up of a number of
passes, driven by the common data structure (Node and PageInof etc).  I'd like
to keep Parser only doing the basic parsing, but leaves the optimizations to
other passes.  Not only will it be easier for debugging and maintenance,
switching on and off such optimizations will also made easier.

Like I said, this no big deal, so I'll leave the patch in for now.  Anyway,
thanks for contributing to improve Jasper, and don't let slight disagreements
like this prevent you from making more contributions in the future.
Comment 6 Eric Carmichael 2003-08-21 06:00:21 UTC
It's just occurred to me that if the consolidation were done in a separate 
pass, it would apply to JSPs in XML format as well.  So provided the 
performance hit during compilation is acceptable (and I take your point that 
webapps should be deployed precompiled), I agree a separate pass is the way to 
go.

But if a performance hit during compilation is acceptable, is there any reason 
not to do this consolidation even when mappedfile="false"?  As you say, it 
should improve runtime performance, and we always have JspUtil.CHUNKSIZE 
(which could be made a configurable parameter) to keep individual lines from 
getting too long.  That was the only other downside that I found when I tested 
with mappedfile="false"-- very long template text sections were arguably even 
harder to follow all crammed into one output line than they were split at 
every HTML tag.