Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(38)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 649113005: Make JSON parsing work as a chunked conversion sink. (Closed)

Created:
6 years, 2 months ago by Lasse Reichstein Nielsen
Modified:
6 years, 1 month ago
CC:
reviews_dartlang.org, vm-dev_dartlang.org, floitsch
Visibility:
Public.

Description

Make JSON parsing work as a chunked conversion sink. This allows JSON parsing to work on individual chunks of source instead of collecting and concatenating all the parts before parsing. This is made in anticipation of having a UTF-8 version that parses directly on the input codes, without creating a String first. R=sgjesse@google.com Committed: https://code.google.com/p/dart/source/detail?r=41311

Patch Set 1 #

Patch Set 2 : Add tests, fix bugs. #

Patch Set 3 : Fix assert position #

Patch Set 4 : Another type-fix #

Patch Set 5 : Also add an UTF-8 base JSON parser, without intermediate string representations. #

Total comments: 50

Patch Set 6 : Address comments. #

Patch Set 7 : Address comments. Fix bug. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2038 lines, -231 lines) Patch
M runtime/lib/convert_patch.dart View 1 2 3 4 5 6 16 chunks +1305 lines, -161 lines 0 comments Download
M runtime/lib/double_patch.dart View 1 chunk +1 line, -27 lines 0 comments Download
M sdk/lib/_internal/compiler/js_lib/convert_patch.dart View 1 chunk +32 lines, -0 lines 0 comments Download
M sdk/lib/convert/json.dart View 1 chunk +1 line, -28 lines 0 comments Download
M sdk/lib/internal/internal.dart View 1 chunk +29 lines, -0 lines 0 comments Download
A tests/lib/convert/json_chunk_test.dart View 1 2 3 4 5 1 chunk +173 lines, -0 lines 0 comments Download
M tests/lib/convert/json_test.dart View 2 chunks +45 lines, -13 lines 0 comments Download
A tests/lib/convert/json_utf8_chunk_test.dart View 1 2 3 4 5 6 1 chunk +450 lines, -0 lines 0 comments Download
M tests/lib/convert/unicode_tests.dart View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
Lasse Reichstein Nielsen
6 years, 2 months ago (2014-10-20 06:38:56 UTC) #2
floitsch
DBC. https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_patch.dart File runtime/lib/convert_patch.dart (right): https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_patch.dart#newcode157 runtime/lib/convert_patch.dart:157: static const int kMinCapacity = 16; constants in ...
6 years, 2 months ago (2014-10-20 08:52:45 UTC) #4
Søren Gjesse
LGTM! https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_patch.dart File runtime/lib/convert_patch.dart (right): https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_patch.dart#newcode413 runtime/lib/convert_patch.dart:413: * state is an error. And the states ...
6 years, 2 months ago (2014-10-24 11:12:25 UTC) #5
Lasse Reichstein Nielsen
Committed patchset #7 manually as r41311 (presubmit successful).
6 years, 1 month ago (2014-10-27 12:11:51 UTC) #6
Lasse Reichstein Nielsen
6 years, 1 month ago (2014-10-27 12:42:34 UTC) #7
Message was sent while issue was closed. 
https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
File runtime/lib/convert_patch.dart (right):

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:157: static const int kMinCapacity = 16;
I know. The style guide changed so they are no longer written as MIN_CAPACITY,
so I wanted something else. I'll change it to just minCapacity.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:175: minCapacity = (minCapacity + 3) & ~3;  //
Round to multile of four.
On 2014/10/20 08:52:44, floitsch wrote:
> multiple

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:180: void ensureCapacity(int newCapcity) {
On 2014/10/20 08:52:44, floitsch wrote:
> newCapacity

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:188: String toString() => "NumberBuffer";
I think I had that for debugging, but I'll just remove the entire toString.
Getting "Instance of _NumberBuffer" is perfectly fine.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:413: * state is an error.
That's what the next paragraph tries to say. I'll make it more clear.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:430: fail(chunkEnd, "Unterminate string");
On 2014/10/24 11:12:24, Søren Gjesse wrote:
> Unterminated

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:451: // Sets the current source chunk.
On 2014/10/20 08:52:44, floitsch wrote:
> Make all these comments dartdocs.

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:457: // Returns the chunk itself. Used by fail to
include it in FormatException.
Yes, that's all we have.
The FormatException will be thrown by the call to
ByteConversionSink.add(List<int> bytes), so it shouldn't be surprising that the
bytes list is the only available context.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:474: // Adds slice of current chunk to string
being built.
On 2014/10/20 08:52:44, floitsch wrote:
> end exclusive?

Acknowledged.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:479: // Extracts a literal string from a source
slice.
On 2014/10/24 11:12:23, Søren Gjesse wrote:
> source slice -> chunk slice

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:486: // Parse a slice of input as an integer.
On 2014/10/24 11:12:24, Søren Gjesse wrote:
> slice of input -> chunk slice

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:492: // Parse a slice of input as a double.
On 2014/10/24 11:12:24, Søren Gjesse wrote:
> ditto.

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:532: // Starts at chunk index 0, and returns the
index of the first
At [position] actually.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:562: if (digit <= 9) return fail(position);
On 2014/10/20 08:52:44, floitsch wrote:
> Add comment, why this is not allowed.

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:945: int parseStringEscape(int position) {
On 2014/10/24 11:12:24, Søren Gjesse wrote:
> Add a comment that position is just after the backslash.

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:1093: intValue -= 1;
It is collecting *an* integer value. 
I'll just document that the intValue field is used for both integer literals,
and the exponent of double literals.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:1103: intValue = 0;
Same.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:1135: const double maxExactDouble =
9007199254740992.0;
On 2014/10/20 08:52:44, floitsch wrote:
> comment.

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:1154: listener.handleNumber(parseDouble(start,
position));
On 2014/10/20 08:52:44, floitsch wrote:
> comment.

Done.

https://codereview.chromium.org/649113005/diff/140001/runtime/lib/convert_pat...
runtime/lib/convert_patch.dart:1294: 
Comment added. We convert all non-BMP characters to surrogate pairs here. That
allows us to use a Uint16List, and there is no need for String.fromCharCodes to
do the same thing.

I don't know if we convert Uint16List to strings incredibly quickly, but we
should - it doesn't need to check the content at all.

https://codereview.chromium.org/649113005/diff/140001/tests/lib/convert/json_...
File tests/lib/convert/json_chunk_test.dart (right):

https://codereview.chromium.org/649113005/diff/140001/tests/lib/convert/json_...
tests/lib/convert/json_chunk_test.dart:12: jsonTest(name, expect, action(sink))
{
testName.

https://codereview.chromium.org/649113005/diff/140001/tests/lib/convert/json_...
tests/lib/convert/json_chunk_test.dart:15: Expect.equals(expect, value,
"$name$value");
On 2014/10/24 11:12:24, Søren Gjesse wrote:
> Separator before $value

Done.

https://codereview.chromium.org/649113005/diff/140001/tests/lib/convert/json_...
tests/lib/convert/json_chunk_test.dart:56: for (var number in ["-0.12e-12",
"-34.12E+12", "0.0e0", "9.9E9"]) {
Adding more numbers.
The important part here is to have splits at all possible number states, but
I'll ensure that we also test numbers without exponent etc.

No, ".1" isn't valid JSON. (I wouldn't say "illegal" - it's just a spec, not a
law! - pet peeve :).

I'll add some negative tests too, to ensure that we don't, e.g., allow leading
zeros across chunk splits.

https://codereview.chromium.org/649113005/diff/140001/tests/lib/convert/json_...
File tests/lib/convert/json_utf8_chunk_test.dart (right):

https://codereview.chromium.org/649113005/diff/140001/tests/lib/convert/json_...
tests/lib/convert/json_utf8_chunk_test.dart:22: void jsonTest(name, expect,
action(sink), [bool allowMalformed = false]) {
On 2014/10/24 11:12:24, Søren Gjesse wrote:
> name -> testName or nameOfTest

Done.

https://codereview.chromium.org/649113005/diff/140001/tests/lib/convert/json_...
tests/lib/convert/json_utf8_chunk_test.dart:24: Expect.equals(expect, value,
"$name$value");
On 2014/10/24 11:12:25, Søren Gjesse wrote:
> Separator before $value.

Done.

Powered by Google App Engine
This is Rietveld 408576698