Author: kkolinko Date: Sat Oct 29 00:55:28 2011 New Revision: 1190720 URL: http://svn.apache.org/viewvc?rev=1190720&view=rev Log: Add one more patch to Mark's proposal. Veto and add review comments.
Modified: tomcat/tc5.5.x/trunk/STATUS.txt Modified: tomcat/tc5.5.x/trunk/STATUS.txt URL: http://svn.apache.org/viewvc/tomcat/tc5.5.x/trunk/STATUS.txt?rev=1190720&r1=1190719&r2=1190720&view=diff ============================================================================== --- tomcat/tc5.5.x/trunk/STATUS.txt (original) +++ tomcat/tc5.5.x/trunk/STATUS.txt Sat Oct 29 00:55:28 2011 @@ -70,7 +70,56 @@ PATCHES PROPOSED TO BACKPORT: * Improve performance of parameter processing http://people.apache.org/~markt/patches/2011-10-27-param-perf-tc5-v1.patch - http://svn.apache.org/viewvc?rev=1190481&view=rev + http://svn.apache.org/viewvc?rev=1190481&view=rev - fixes NPE + http://svn.apache.org/viewvc?rev=1190371&view=rev - make ByteChunk.DEFAULT_CHARSET final +1: markt - -1: + -1: kkolinko: because of ByteChunk#toStringInternal(), see below. + kkolinko: In B2CConverter.java + lines 117-118: restore Javadoc comment to method void convert( ByteChunk bb, CharChunk cb, int limit) + line 125 - remove "// conv.ready() ) {" comment. It wasn't in 5.5. Do not see why to add it. + In ByteChunk.java: + in toStringInternal(): + "cb.array()" returns "character array that backs this CharBuffer". + I think it can contain extra characters beyond end of decoded string. + Thus "new String(cb.array());" is wrong. + Single-character charsets are simple, because the count of characters + is known from the count of bytes. Needs more testing with UTF-8. + - thus -1. + In Parameters.java: + "private static org.apache.commons.logging.Log log" + - make "static final" + "StringManager.getManager("org.apache.tomcat.util.http");" + - the LocalStrings.properties file from r1189882 is not included in this patch. + (2011-10-27-param-perf-tc6-v1.patch does not have it as well) + "private final Hashtable paramHashValues" + - Maybe a HashMap can be used instead. I do not expect much improvements + from that though. + in #addParameterValues(..) + - Replace "values = new ArrayList(1);" + with "values = new ArrayList(newValues.length);" + - Maybe "if (paramHashValues.containsKey(key))" can be replaced + with testing whether result of (paramHashValues.containsKey(key)) is null. + - Maybe add tests for this method. In trunk it is called by Request.parseParts(), though see below maybe that can be changed. + in #getParameterValues(String name) + - Apply NPE fix from http://svn.apache.org/viewvc?rev=1190481&view=rev + - Maybe add test case in TestParameters.java for calling getParameterValues() for non-existing parameter. + in #addParam(String, String) + - remove efficiency comment above the method? + - In trunk: maybe make this method public and call it instead of #addParameterValues(..) + in Request.parseParts(). + - Maybe "if (paramHashValues.containsKey(key))" can be replaced + with testing whether result of (paramHashValues.containsKey(key)) is null. + "public static final String DEFAULT_ENCODING" + "public static final Charset DEFAULT_CHARSET" + - New fields. They can be made private. + in #processParameters(..., Charset) + - It is a new method. Maybe make it private. + - If it remains public, maybe document that charset parameter is not null. + All callers here call Parameters.getCharset(encoding) which returns DEFAULT_CHARSET by default. + in #urlDecode(...) + - Maybe call "bc.setCharset(charset);" before calling "urlDec.convert(bc);" + in #paramsAsString() + - Move sb.append("\n"); outside of loop that iterates on values. Those are separated by ','. + In the old code the "\n" is used to separate different parameter names. + - Replace double quotes with single quotes where it is a single character ('=', ',', '\n') --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org