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: [email protected]
For additional commands, e-mail: [email protected]
