[gwt-contrib] Re: Avoid Java bottleneck by using explicit Charset for byte[]-String conversions (issue1588803)

[rdayal]

On 2012/01/23 23:43:12, rdayal wrote:

LGTM.


Submitted.

http://gwt-code-reviews.appspot.com/1588803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Avoid Java bottleneck by using explicit Charset for byte[]-String conversions (issue1588803)

[rdayal]

LGTM.

http://gwt-code-reviews.appspot.com/1588803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Avoid Java bottleneck by using explicit Charset for byte[]-String conversions (issue1588803)

[t . broyer]

LGTM


http://gwt-code-reviews.appspot.com/1588803/diff/11001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java
File src/com/google/gwt/user/server/rpc/RPCServletUtils.java (right):

http://gwt-code-reviews.appspot.com/1588803/diff/11001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java#newcode74
src/com/google/gwt/user/server/rpc/RPCServletUtils.java:74: private
static ConcurrentHashMapString, Charset CHARSET_CACHE =
Just noticed it: should this field be 'final'?

http://gwt-code-reviews.appspot.com/1588803/diff/11001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java#newcode124
src/com/google/gwt/user/server/rpc/RPCServletUtils.java:124: *
  the default UTF-8 character set will be returned.
There probably should be an explicit unit test added to
RPCServletUtilsTest; something like:
assertSame(RPCServletUtils.CHARSET_UTF8,
RPCServletUtils.getCharset(null));

And also add a few tests checking all calls to getCharset return the
same instance (it's obvious the tests would be green –though that would
have caught the s/encoding/charset/ bug you fixed below, that Rajeev and
I hadn't noticed– but they'd be great regression tests)

http://gwt-code-reviews.appspot.com/1588803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Avoid Java bottleneck by using explicit Charset for byte[]-String conversions (issue1588803)

[rdayal]

Thanks for doing this!

A few comments.


http://gwt-code-reviews.appspot.com/1588803/diff/2001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java
File src/com/google/gwt/user/server/rpc/RPCServletUtils.java (right):

http://gwt-code-reviews.appspot.com/1588803/diff/2001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java#newcode70
src/com/google/gwt/user/server/rpc/RPCServletUtils.java:70: private
static ConcurrentHashMapString, Charset CHARSET_CACHE =
Maybe have a private static method that creates this map and adds
CHARSET_UTF8 as the first entry?

Suggestion: You could also map CHARSET_UTF8 to the null key; that would
clean up some of the ternary ifs at other points in the code.

http://gwt-code-reviews.appspot.com/1588803/diff/2001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java#newcode118
src/com/google/gwt/user/server/rpc/RPCServletUtils.java:118: charset =
Charset.forName(encoding);
Do we want an extra level of locking here (to prevent multiple threads
attempting to use Charset.forName(...)), or do you feel that in this
case, the contention is acceptable?

http://gwt-code-reviews.appspot.com/1588803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Avoid Java bottleneck by using explicit Charset for byte[]-String conversions (issue1588803)

[t . broyer]

Given that only UTF-8 is used, how about an eagerly-initialized
RPCServletUtil.UTF8_CHARSET instead? (similar to Guava's Charsets.UTF_8
[1])

[1]
http://docs.guava-libraries.googlecode.com/git-history/v10.0.1/javadoc/com/google/common/base/Charsets.html#UTF_8

http://gwt-code-reviews.appspot.com/1588803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Avoid Java bottleneck by using explicit Charset for byte[]-String conversions (issue1588803)

[t . broyer]

LGTM, but I'm not a Googler sorry. You'll have to find a reviewer and
committer.


http://gwt-code-reviews.appspot.com/1588803/diff/2001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java
File src/com/google/gwt/user/server/rpc/RPCServletUtils.java (right):

http://gwt-code-reviews.appspot.com/1588803/diff/2001/src/com/google/gwt/user/server/rpc/RPCServletUtils.java#newcode379
src/com/google/gwt/user/server/rpc/RPCServletUtils.java:379:
response.getOutputStream().write(GENERIC_FAILURE_MSG.getBytes(getCharset(UTF-8)));
CHARSET_UTF8 instead?

http://gwt-code-reviews.appspot.com/1588803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors