Improve JSONReader performance by up to 55% by using std::string instead of wstring.

Improve JSONReader performance by up to 55% by using std::string instead of wstring.

Before this change, JSONReader would:
1. Take std::string input
2. Convert it to wstring
3. Parse
4. Decode strings for the object representation, converting wstring to string16
5. Create a base::Value with a string16, which internally converts back to std::string

After this change, JSONReader does:
1. Take std::string input
2. Parse
3. Create a base::Value with a std::string

BUG=111581
TEST=Covered by unittests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128678

[Robert Sesek, 2012-03-21 19:23:53]

willchan: review please
jshin: I was told you have familiarity with ICU and unicode. Would you mind
reviewing the DecodeString() function?

[willchan no longer on Chromium, 2012-03-22 00:51:10]

Wow, I know very little about std::string vs wsting vs string16, not to mention
unicode/icu. Punting to Brett.

[brettw, 2012-03-22 05:51:42]

https://chromiumcodereview.appspot.com/9801007/diff/14001/base/json/json_read...
File base/json/json_reader.cc (right):

https://chromiumcodereview.appspot.com/9801007/diff/14001/base/json/json_read...
base/json/json_reader.cc:491: // Consume the UTF-16 code unit, which may be a
high surrogate.
This case is getting kind of crazy long, can you separate this one into a
ReadUnicodeChar (or something) function?

https://chromiumcodereview.appspot.com/9801007/diff/14001/base/json/json_read...
base/json/json_reader.cc:511: return NULL;
You changed the behavior of this function by failing for different encoding
issues. Before it looks like it wouldn't fail. Are we sure we want to change
this? (It's fine if we do, I just want to make sure we understand everything.)

It also seems weird to me that this code fails for things like surrogate
problems, but not for things like \u$%@! (which will be 0).

https://chromiumcodereview.appspot.com/9801007/diff/14001/base/json/json_read...
base/json/json_reader.cc:521: (HexDigitToInt(*(token.begin + i + 1)) << 12) +
It's sort of annoying that this is duplicated twice. I wonder if another helper
function (and if we decide to be strict, it would check for errors) is in order.

[brettw, 2012-03-22 05:51:50]

BTW this is awesome!

[Robert Sesek, 2012-03-22 17:59:57]

http://codereview.chromium.org/9801007/diff/14001/base/json/json_reader.cc
File base/json/json_reader.cc (right):

http://codereview.chromium.org/9801007/diff/14001/base/json/json_reader.cc#ne...
base/json/json_reader.cc:491: // Consume the UTF-16 code unit, which may be a
high surrogate.
On 2012/03/22 05:51:42, brettw wrote:
> This case is getting kind of crazy long, can you separate this one into a
> ReadUnicodeChar (or something) function?

Done.

http://codereview.chromium.org/9801007/diff/14001/base/json/json_reader.cc#ne...
base/json/json_reader.cc:511: return NULL;
On 2012/03/22 05:51:42, brettw wrote:
> You changed the behavior of this function by failing for different encoding
> issues. Before it looks like it wouldn't fail. Are we sure we want to change
> this? (It's fine if we do, I just want to make sure we understand everything.)

I think we do. The JSON spec states what's legal in a string, and I think
invalid escape sequences constitute invalid JSON. I think failure is the right
approach here.

> It also seems weird to me that this code fails for things like surrogate
> problems, but not for things like \u$%@! (which will be 0).

This will fail for that invalid code unit (added it to the unittest). Line 530
will validate that.

http://codereview.chromium.org/9801007/diff/14001/base/json/json_reader.cc#ne...
base/json/json_reader.cc:521: (HexDigitToInt(*(token.begin + i + 1)) << 12) +
On 2012/03/22 05:51:42, brettw wrote:
> It's sort of annoying that this is duplicated twice. I wonder if another
helper
> function (and if we decide to be strict, it would check for errors) is in
order.

Done. Can you explain what you mean by "be strict"? The first parse phase
validates that these digits are HEX.

[brettw, 2012-03-22 20:23:23]

http://codereview.chromium.org/9801007/diff/22001/base/json/json_reader.cc
File base/json/json_reader.cc (right):

http://codereview.chromium.org/9801007/diff/22001/base/json/json_reader.cc#ne...
base/json/json_reader.cc:566: return (HexDigitToInt(*(buf + 1)) << 12) +
You didn't fix the general problem, though, which is the current HexDigitToInt
doesn't report errors. What about \uzz89 which will be 0x89 which would pass
your test.

[Robert Sesek, 2012-03-22 20:52:16]

http://codereview.chromium.org/9801007/diff/22001/base/json/json_reader.cc
File base/json/json_reader.cc (right):

http://codereview.chromium.org/9801007/diff/22001/base/json/json_reader.cc#ne...
base/json/json_reader.cc:566: return (HexDigitToInt(*(buf + 1)) << 12) +
On 2012/03/22 20:23:23, brettw wrote:
> You didn't fix the general problem, though, which is the current HexDigitToInt
> doesn't report errors. What about \uzz89 which will be 0x89 which would pass
> your test.

That also caused the parser to fail, but I see your point. Switched to using
HexStringToInt()

[Mark Mentovai, 2012-03-22 21:06:20]

http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc
File base/json/json_reader.cc (right):

http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc#ne...
base/json/json_reader.cc:487: decoded_str.push_back((HexDigitToInt(*(token.begin
+ i + 1)) << 4) +
HexStringToInt here too?

http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc#ne...
base/json/json_reader.cc:488: HexDigitToInt(*(token.begin + i + 2)));
All of this +1, +2, +3 stuff seems kind of unsafe.

[Robert Sesek, 2012-03-23 15:51:41]

On 2012/03/22 21:06:20, Mark Mentovai wrote:
> http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc
> File base/json/json_reader.cc (right):
> 
>
http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc#ne...
> base/json/json_reader.cc:487:
decoded_str.push_back((HexDigitToInt(*(token.begin
> + i + 1)) << 4) +
> HexStringToInt here too?
> 
>
http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc#ne...
> base/json/json_reader.cc:488: HexDigitToInt(*(token.begin + i + 2)));
> All of this +1, +2, +3 stuff seems kind of unsafe.

I agree that these should be changed, but I want to keep this CL scoped small.
I'd like to have high confidence in the change I'm making because I have pretty
low confidence in the parser overall (many cases don't look properly checked).
This CL is really just a bandaid to get a nice speed bump in M19, because for
M20 (i.e. like next week) I intend to rewrite this properly to 1) run in O(n)
time, 2) operate more safely with memory, and 3) use StringPiece to reduce some
memory copies.

[brettw, 2012-03-23 16:29:24]

http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc
File base/json/json_reader.cc (right):

http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc#ne...
base/json/json_reader.cc:488: HexDigitToInt(*(token.begin + i + 2)));
Let's do Mark's suggestion. It's minor compared to the risk of the rest of this
patch, and this code is clearly wrong (not checking the bounds or hex digit
validity). Lots of times people don't get around to the refactoring they mean to
get to, so I'd rather not put off this minor fix.

[Robert Sesek, 2012-03-23 18:40:27]

http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc
File base/json/json_reader.cc (right):

http://codereview.chromium.org/9801007/diff/25003/base/json/json_reader.cc#ne...
base/json/json_reader.cc:488: HexDigitToInt(*(token.begin + i + 2)));
On 2012/03/23 16:29:24, brettw wrote:
> Let's do Mark's suggestion. It's minor compared to the risk of the rest of
this
> patch, and this code is clearly wrong (not checking the bounds or hex digit
> validity). Lots of times people don't get around to the refactoring they mean
to
> get to, so I'd rather not put off this minor fix.

Fine, done.

If it was wrong for 18 releases, I don't see how one more would hurt though ;).

[brettw, 2012-03-23 19:10:49]

lgtm

[commit-bot: I haz the power, 2012-03-23 21:43:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/9801007/21009

[commit-bot: I haz the power, 2012-03-23 23:31:25]

Try job failure for 9801007-21009 (retry) on mac_rel for step "update".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-03-24 00:44:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/9801007/21009

[commit-bot: I haz the power, 2012-03-24 03:57:18]

Change committed as 128678