[json-parse-with-source] Implement the JSON.parse source text [v8/v8 : main]

王澳 (Gerrit)

unread,

Sep 22, 2022, 11:48:46 PM9/22/22

to victorgo...@chromium.org, Shu-yu Guo, Marja Hölttä, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Shu-yu Guo.

View Change

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Sep 27, 2022, 7:16:06 AM9/27/22

to 王澳, victorgo...@chromium.org, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo, 王澳.

View Change

13 comments:

Patchset:
- Patch Set #12:
  Thanks for working on this! The first round of comments below. I'll still need to read the spec in more detail to give more detailed comments...
File src/json/json-parser.h:
- Patch Set #12, Line 169: using SmallVector = base::SmallVector<T, 16>;
  Ha, this is confusing, I was already wondering why you're omitting the size of the SmallVector and why this compiles!
File src/json/json-parser.cc:
- Patch Set #12, Line 134: if (!
  Here and below (but not above, this doesn't apply to InitializeGlobal_harmony_json_parse_with_source)
  I think these would be clearer if you always wrote the "with feature" part first.
```
if (feature_enabled) {
 ...
} else {
 ...
}
```
  -> less mental churn for the reader, since they're always the same way around.
- Patch Set #12, Line 174: if
  Style nit: when the if clause body doesn't fit on the same line, please add { }
- Patch Set #12, Line 915:
```
    case JsonToken::LBRACE:
    case JsonToken::LBRACK:
    case JsonToken::COLON:
    case JsonToken::COMMA:
    case JsonToken::ILLEGAL:
    case JsonToken::RBRACE:
    case JsonToken::RBRACK:
    case JsonToken::EOS:
    case JsonToken::WHITESPACE:
```
  Could this be:
  default:
- Patch Set #12, Line 948: val_node
  Could you avoid the val_node bookkeeping when there's no custom reviver? In that case, we don't need to do the extra work of keeping track of the 'source'.
  This should make sure we don't regress the performance of doing JSON.parse("...");
- Patch Set #12, Line 948:
```
  Handle<Object> val_node;
  int start_position;
  int end_position;
  Handle<FixedArray> pos;
  SmallVector<Handle<Object>> element_val_node_stack;
  SmallVector<Handle<Object>> property_val_node_stack;
```
  You could add a comment here describing how the val node stack handling works.
- Patch Set #12, Line 1055:
```
if (v8_flags.harmony_json_parse_with_source) {
        end_position = position();
        pos->set(0, Smi::FromInt(start_position));
        pos->set(1, Smi::FromInt(end_position));
        val_node = pos;
      }
```
  Why is this needed? (A comment would be helpful here.)
- Patch Set #12, Line 1131:
```
Handle<ObjectHashTable> table =
                ObjectHashTable::New(isolate(), length);
```
  I'm a bit lost here, why do we need an ObjectHashTable? Even in a object parsed from JSON, there's a natural ordering of the property keys and property values, why not use arrays?
File test/mjsunit/harmony/json-parse-with-source.js:
- Patch Set #12, Line 22: { source }
  As a separate test, you could add a test testing in more detail what kind of object this 'context' object passed to the reviver is (see below for another similar suggestion).
  In addition, here you don't differentiate between the case where there's no property called "source" and the case where there is, but its value is undefined, could you also add a test asserting that the 'source' property is missing in the case where it's supposed to be missing?
- Patch Set #12, Line 106: undefined
  You could add a comment about what the 'undefined's are
- Patch Set #12, Line 159: TestRawJson
  These tests would be interesting:
  Test what kind of an object we get back from JSON.rawJSON(), e.g., that its `__proto__` is Object.prototype, that it only has the rawJSON property etc.
- Patch Set #12, Line 184: assertThrows
  It would make sense to always add the error type to assertThrows, this guards against a bug where something throws for a different reason than what was intended. (Doesn't fully make that bug impossible, but in some cases it's possible to catch it.)

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Sep 27, 2022, 7:26:38 AM9/27/22

to 王澳, victorgo...@chromium.org, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo, 王澳.

View Change

1 comment:

Patchset:
- Patch Set #12:
  Could you also rebase and kick of try jobs? Thanks!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Sep 29, 2022, 4:53:44 AM9/29/22

to 王澳, victorgo...@chromium.org, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo, 王澳.

View Change

2 comments:

File src/json/json-parser.cc:
- Patch Set #12, Line 124: Internalize
  General question: if you turn on your flag locally, do all existing tests pass?

Patch Set #12, Line 1055:

if (v8_flags.harmony_json_parse_with_source) {
        end_position = position();
        pos->set(0, Smi::FromInt(start_position));
        pos->set(1, Smi::FromInt(end_position));
        val_node = pos;
      }

- Why is this needed? (A comment would be helpful here. […]
  Ah, this is so that we create get the correct substring later. Please add a comment describing what's in val_node in various points in time!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Sep 29, 2022, 9:44:24 AM9/29/22

to victorgo...@chromium.org, Shu-yu Guo, Marja Hölttä, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Shu-yu Guo.

Patch set 15:Commit-Queue +1

View Change

13 comments:

Patchset:
- Patch Set #15:
  Thanks for the review!
File src/json/json-parser.cc:

- Patch Set #12, Line 124: Internalize
  General question: if you turn on your flag locally, do all existing tests pass?

Hi Marja, not all the existing tests pass with turning on the flag locally. But I think it's a bug in the spec. I file a issue [here](https://github.com/tc39/proposal-json-parse-with-source/issues/35). E.g., the test below in `test/mjsunit/json.js` failed.
```
reviver = function(p, v) {
  if (p == "a") {
    this.b = { get x() {return null}, set x(_){throw 666} }
  }
  return v;
}
assertEquals({a: 0, b: {x: null}}, JSON.parse('{"a":0,"b":1}', reviver));

```
The spec will assert the typedValNode is ObjectLiteral Parse Node when calling InternalizeJSONProperty for the `b` property, but it's NumberLiteral Parse Node[1] actually.

- Patch Set #12, Line 134: if (!

- Here and below (but not above, this doesn't apply to InitializeGlobal_harmony_json_parse_with_source […]
  Done

- Patch Set #12, Line 174: if
  Style nit: when the if clause body doesn't fit on the same line, please add { }

- Done

Patch Set #12, Line 915:

    case JsonToken::LBRACE:
    case JsonToken::LBRACK:
    case JsonToken::COLON:
    case JsonToken::COMMA:
    case JsonToken::ILLEGAL:
    case JsonToken::RBRACE:
    case JsonToken::RBRACK:
    case JsonToken::EOS:
    case JsonToken::WHITESPACE:

- Could this be: […]
  Done

- Patch Set #12, Line 948: val_node

- Could you avoid the val_node bookkeeping when there's no custom reviver? In that case, we don't need […]
  Done. Thanks!

Patch Set #12, Line 948:

  Handle<Object> val_node;
  int start_position;
  int end_position;
  Handle<FixedArray> pos;
  SmallVector<Handle<Object>> element_val_node_stack;
  SmallVector<Handle<Object>> property_val_node_stack;

You could add a comment here describing how the val node stack handling works.

- Done

Patch Set #12, Line 1055:

if (v8_flags.harmony_json_parse_with_source) {
        end_position = position();
        pos->set(0, Smi::FromInt(start_position));
        pos->set(1, Smi::FromInt(end_position));
        val_node = pos;
      }

- Ah, this is so that we create get the correct substring later. […]
  Done

Patch Set #12, Line 1131:

Handle<ObjectHashTable> table =
                ObjectHashTable::New(isolate(), length);

- I'm a bit lost here, why do we need an ObjectHashTable? Even in a object parsed from JSON, there's a […]
  Because for JSObject values, the order in which properties are defined may be different from the order in which properties are enumerated when calling InternalizeJSONProperty for the JSObject value. E.g., the json source string is '{"a": 1, "1": 2}', and the properties enumerate order is ["1", "a"]. Moreover, properties may be defined repeatedly in the json string. E.g., the json string is '{"a": 1, "a": 1}', and the properties enumerate order is ["a"]. So I use a ObjectHashTable here to record the property name and the property's parse node, and then look up the property's parse node by the property name when calling InternalizeJSONProperty. WDYT? Thanks!
File test/mjsunit/harmony/json-parse-with-source.js:

- Patch Set #12, Line 22: { source }

- As a separate test, you could add a test testing in more detail what kind of object this 'context' o […]
  Done

- Patch Set #12, Line 106: undefined
  You could add a comment about what the 'undefined's are

- Done

- Patch Set #12, Line 159: TestRawJson

- These tests would be interesting: […]
  Done

- Patch Set #12, Line 184: assertThrows

- It would make sense to always add the error type to assertThrows, this guards against a bug where so […]
  Done

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Sep 29, 2022, 10:08:36 AM9/29/22

to victorgo...@chromium.org, Shu-yu Guo, Marja Hölttä, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Shu-yu Guo.

Patch set 15:-Commit-Queue

View Change

1 comment:

Patchset:

- Patch Set #12:
  Could you also rebase and kick of try jobs? Thanks!

- Done. Thanks!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Oct 4, 2022, 8:55:57 AM10/4/22

to victorgo...@chromium.org, Shu-yu Guo, Marja Hölttä, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Shu-yu Guo.

View Change

1 comment:

Patchset:
- Patch Set #20:
  Gentle ping to PTAL. Thanks!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Oct 5, 2022, 2:12:11 AM10/5/22

to 王澳, victorgo...@chromium.org, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo, 王澳.

View Change

1 comment:

Patchset:
- Patch Set #20:
  Gentle ping to PTAL. […]
  Sorry for the delay, I'll do another code review round this week! Thanks!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Oct 5, 2022, 3:38:31 AM10/5/22

to 王澳, victorgo...@chromium.org, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo, 王澳.

Patch set 20:Code-Review +1

View Change

9 comments:

Patchset:
- Patch Set #20:
  lgtm, thanks!

File src/common/message-template.h:

Patch Set #20, Line 115:

T(InvalidArgument, "invalid_argument")                                       \
  T(InvalidArgumentForTemporal, "Invalid argument for Temporal %")             \
  T(InvalidInOperatorUse, "Cannot use 'in' operator to search for '%' in %")   \
  T(InvalidRegExpExecResult,                                                   \
    "RegExp exec method returned something other than an Object or null")      \
  T(InvalidUnit, "Invalid unit argument for %() '%'")                          \

Around here would be a good place for the added error msg

Patch Set #20, Line 705: T(InvalidRawJsonValue, "Invalid value for JSON.rawJSON")
This could be moved up where the other TypeErrors are, I don't think we need a separate section for this feature as there's only one error message.

File src/json/json-parser.cc:

- Patch Set #12, Line 124: Internalize

- Hi Marja, not all the existing tests pass with turning on the flag locally. […]
  Ok, good to know that early!

Patch Set #12, Line 1131:

Handle<ObjectHashTable> table =
                ObjectHashTable::New(isolate(), length);

- Because for JSObject values, the order in which properties are defined may be different from the ord […]
  Ack; I also saw you added a comment explaining that above, thanks!
File src/json/json-parser.cc:
- Patch Set #20, Line 135: DCHECK(result->IsFixedArray());
  You could also DCHECK the length
- Patch Set #20, Line 148: InternalizeJsonProperty
  This (and other "Internalize" funcs are only called when the reviver is callable, right? Could you add DCHECK(reviver_->IsCallable()); to make that clearer?
- Patch Set #20, Line 946: containing all the elements's parse node
  Nit: Better would be: "containing the parse nodes of the elements", or if you want it this way, then "containing all the elements' parse nodes".
- Patch Set #20, Line 984: v8_flags.harmony_json_parse_with_source && reviver_is_callable)
  You could create one bool which stores this value, something like: should_track_json_source or something along those lines (maybe you have a better idea for a name?)

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Oct 5, 2022, 3:40:45 AM10/5/22

to 王澳, victorgo...@chromium.org, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo, 王澳.

View Change

1 comment:

File src/json/json-parser.cc:

- Patch Set #12, Line 124: Internalize

- Ok, good to know that early!
  Hmm, now that I'm thinking... so you didn't have test coverage for this case (reviver modifying the thing), after this has been sorted out, I think it would be good to add test coverage for the "source text + reviver modifications" combo.
  Also could you add a comment into the source code with a link to the bug?

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Oct 5, 2022, 12:17:09 PM10/5/22

to victorgo...@chromium.org, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Shu-yu Guo.

Patch set 21:Commit-Queue +1

View Change

8 comments:

Patchset:
- Patch Set #21:
  Thanks for the review!
File src/common/message-template.h:

Patch Set #20, Line 115:

T(InvalidArgument, "invalid_argument")                                       \
  T(InvalidArgumentForTemporal, "Invalid argument for Temporal %")             \
  T(InvalidInOperatorUse, "Cannot use 'in' operator to search for '%' in %")   \
  T(InvalidRegExpExecResult,                                                   \
    "RegExp exec method returned something other than an Object or null")      \
  T(InvalidUnit, "Invalid unit argument for %() '%'")                          \

Around here would be a good place for the added error msg

- Done

- Patch Set #20, Line 705: T(InvalidRawJsonValue, "Invalid value for JSON.rawJSON")

- This could be moved up where the other TypeErrors are, I don't think we need a separate section for […]
  Done
File src/json/json-parser.cc:

- Patch Set #12, Line 124: Internalize

- Hmm, now that I'm thinking... […]
  Done. Thanks! I added some tests for the "source text + reviver modifications" combo in `TestReviverModifyJsonValue` in `test/mjsunit/harmony/json-parse-with-source.js`. And I will add more tests after the [issue](https://github.com/tc39/proposal-json-parse-with-source/issues/35) is resolved. WDYT? Thanks!
File src/json/json-parser.cc:

- Patch Set #20, Line 135: DCHECK(result->IsFixedArray());
  You could also DCHECK the length

- Done

- Patch Set #20, Line 148: InternalizeJsonProperty

- This (and other "Internalize" funcs are only called when the reviver is callable, right? Could you a […]
  Done

- Patch Set #20, Line 946: containing all the elements's parse node

- Nit: Better would be: "containing the parse nodes of the elements", or if you want it this way, then […]
  Done

- Patch Set #20, Line 984: v8_flags.harmony_json_parse_with_source && reviver_is_callable)

- You could create one bool which stores this value, something like: should_track_json_source or somet […]
  Done

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Oct 5, 2022, 12:24:41 PM10/5/22

to victorgo...@chromium.org, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Shu-yu Guo.

Patch set 21:-Commit-Queue

View Change

1 comment:

Patchset:
- Patch Set #21:
  Hi, Marja, I added some comments in `mjsunit/json.js` which is newly modified and the approval got outdated. Would you mind voting again if you think the CL is ok? Thanks a lot!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Oct 6, 2022, 3:25:49 AM10/6/22

to 王澳, victorgo...@chromium.org, Igor Sheludko, Maya Lekova, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Igor Sheludko, Maya Lekova, Shu-yu Guo, 王澳.

Patch set 21:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #21:
  lgtm again
  You'll need to rebase bytecode expectations:
  gm x64.release.generate-bytecode-expectations && out/x64.release/generate-bytecode-expectations --rebaseline
  Also more OWNERS reviews are needed, adding reviewers:
  json -> ishell
  compiler -> mslekova
File test/mjsunit/json.js:
- Patch Set #21, Line 492: // TODO(v8:12955): JSON parse with source access will assert failed when the reviver modifies the json value like this. See https://github.com/tc39/proposal-json-parse-with-source/issues/35.
  Style nit: please wrap to 80 chars (sadly the auto-formatter won't do that)

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Maya Lekova (Gerrit)

unread,

Oct 6, 2022, 5:57:26 AM10/6/22

to 王澳, victorgo...@chromium.org, Igor Sheludko, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Igor Sheludko, Shu-yu Guo, 王澳.

Patch set 21:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #21:
  LGTM for src/compiler, thanks!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Oct 6, 2022, 6:28:03 AM10/6/22

to victorgo...@chromium.org, Maya Lekova, Igor Sheludko, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Igor Sheludko, Marja Hölttä, Maya Lekova, Shu-yu Guo.

Patch set 22:Commit-Queue +1

View Change

2 comments:

Patchset:
- Patch Set #21:
  lgtm again […]
  Done. Thanks!
File test/mjsunit/json.js:

- Patch Set #21, Line 492: // TODO(v8:12955): JSON parse with source access will assert failed when the reviver modifies the json value like this. See https://github.com/tc39/proposal-json-parse-with-source/issues/35.
  Style nit: please wrap to 80 chars (sadly the auto-formatter won't do that)

- Done

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Igor Sheludko (Gerrit)

unread,

Oct 6, 2022, 1:00:09 PM10/6/22

to 王澳, victorgo...@chromium.org, Maya Lekova, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Maya Lekova, Shu-yu Guo, 王澳.

View Change

3 comments:

File src/json/json-parser.cc:
- Patch Set #22, Line 979: should_track_json_source
  I'm afraid checking for these values in a loop might regress the default case without reviver. I'll start a pinpoint job to check if that's the case.
  What might probably help is to make this a template argument and do this check once outside and then dispatch to respective implementation.
File src/objects/js-raw-json-inl.h:
- Patch Set #22, Line 3: // found in the LICENSE file.
  Ditto.
File src/objects/js-raw-json.h:
- Patch Set #22, Line 3: // found in the LICENSE file.
  Please add an empty line after the header comment.

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Pinpoint perf (Gerrit)

unread,

Oct 6, 2022, 1:46:33 PM10/6/22

to 王澳, victorgo...@chromium.org, Maya Lekova, Igor Sheludko, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Maya Lekova, Shu-yu Guo, 王澳.

📍 Job complete.

See results at: https://pinpoint-dot-chromeperf.appspot.com/job/106bc76d1a0000

View Change

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Pinpoint perf (Gerrit)

unread,

Oct 6, 2022, 2:18:01 PM10/6/22

to 王澳, victorgo...@chromium.org, Maya Lekova, Igor Sheludko, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Maya Lekova, Shu-yu Guo, 王澳.

📍 Job complete.

See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15a8b20f1a0000

View Change

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Pinpoint perf (Gerrit)

unread,

Oct 6, 2022, 7:43:12 PM10/6/22

to 王澳, victorgo...@chromium.org, Maya Lekova, Igor Sheludko, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Maya Lekova, Shu-yu Guo, 王澳.

📍 Job complete.

See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12f43b991a0000

View Change

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Pinpoint perf (Gerrit)

unread,

Oct 6, 2022, 8:46:55 PM10/6/22

to 王澳, victorgo...@chromium.org, Maya Lekova, Igor Sheludko, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Maya Lekova, Shu-yu Guo, 王澳.

📍 Job complete.

See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17c10cf31a0000

View Change

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Oct 7, 2022, 12:16:29 AM10/7/22

to victorgo...@chromium.org, Pinpoint perf, Maya Lekova, Igor Sheludko, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Igor Sheludko, Marja Hölttä, Maya Lekova, Shu-yu Guo.

View Change

3 comments:

File src/json/json-parser.cc:

- Patch Set #22, Line 979: should_track_json_source

- I'm afraid checking for these values in a loop might regress the default case without reviver. […]
  Done. Thanks! I updated the CL to use the template argument to do this check once.
File src/objects/js-raw-json-inl.h:

- Patch Set #22, Line 3: // found in the LICENSE file.
  Ditto.

- Done
File src/objects/js-raw-json.h:

- Patch Set #22, Line 3: // found in the LICENSE file.
  Please add an empty line after the header comment.

- Done

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Igor Sheludko (Gerrit)

unread,

Oct 7, 2022, 6:59:59 AM10/7/22

to 王澳, victorgo...@chromium.org, Pinpoint perf, Maya Lekova, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Maya Lekova, Shu-yu Guo, 王澳.

Patch set 25:Code-Review +1

View Change

4 comments:

Patchset:
- Patch Set #25:
  lgtm with nits and suggestions
File src/json/json-parser.cc:
- Patch Set #25, Line 153: MaybeHandle<Object> maybe_val_node
  I think it'll simplify things if you pass it around as `Handle<Object>`.
- Patch Set #25, Line 249: NewStringFromAsciiChecked("source")
  source_string()
- Patch Set #25, Line 958:
```
the val_node is a FixedArray containing the json value's start
  // position and end postion in the source string, so we can lazily create the
  // context's "source" property according to the start and end positions when
  // invoking the reviver
```
  Given that `v8_flags.string_slices` is enabled by default you might want to add a TODO to consider creating a substring right away instead of doing that lazily in InternalizeJsonProperty. If you switch to strings then you can also avoid creating a FixedArray for the true/false/null cases and use respective root strings.

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Oct 7, 2022, 8:08:43 AM10/7/22

to victorgo...@chromium.org, Igor Sheludko, Pinpoint perf, Maya Lekova, Marja Hölttä, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Marja Hölttä, Maya Lekova, Shu-yu Guo.

View Change

4 comments:

Patchset:
- Patch Set #26:
  Hi, Marja, Mslekova, I updated the bytecode expectations before and the approval got outdated. Could you please vote again? Thanks a lot!
File src/json/json-parser.cc:

- Patch Set #25, Line 153: MaybeHandle<Object> maybe_val_node
  I think it'll simplify things if you pass it around as `Handle<Object>`.

- Done

- Patch Set #25, Line 249: NewStringFromAsciiChecked("source")
  source_string()

- Done

Patch Set #25, Line 958:

the val_node is a FixedArray containing the json value's start
  // position and end postion in the source string, so we can lazily create the
  // context's "source" property according to the start and end positions when
  // invoking the reviver

- Given that `v8_flags. […]
  Done. Thanks!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Marja Hölttä (Gerrit)

unread,

Oct 7, 2022, 8:17:40 AM10/7/22

to 王澳, victorgo...@chromium.org, Igor Sheludko, Pinpoint perf, Maya Lekova, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Maya Lekova, Shu-yu Guo, 王澳.

Patch set 26:Code-Review +1

View Change

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Maya Lekova (Gerrit)

unread,

Oct 10, 2022, 2:00:34 AM10/10/22

to 王澳, victorgo...@chromium.org, Marja Hölttä, Igor Sheludko, Pinpoint perf, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo, 王澳.

Patch set 26:Code-Review +1

View Change

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

王澳 (Gerrit)

unread,

Oct 10, 2022, 2:19:35 AM10/10/22

to victorgo...@chromium.org, Maya Lekova, Marja Hölttä, Igor Sheludko, Pinpoint perf, Shu-yu Guo, V8 LUCI CQ, Michael Hablich, v8-re...@googlegroups.com

Attention is currently required from: Shu-yu Guo.

Patch set 26:Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #26:
  Thanks!

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

V8 LUCI CQ (Gerrit)

unread,

Oct 10, 2022, 3:34:01 AM10/10/22

to 王澳, victorgo...@chromium.org, Maya Lekova, Marja Hölttä, Igor Sheludko, Pinpoint perf, Shu-yu Guo, Michael Hablich, v8-re...@googlegroups.com

V8 LUCI CQ submitted this change.

View Change

Approvals:
  Marja Hölttä: Looks good to me
  Maya Lekova: Looks good to me
  Igor Sheludko: Looks good to me
  王澳: Commit

[json-parse-with-source] Implement the JSON.parse source text

... access proposal.

Bug: v8:12955
Change-Id: I339c4ee1849c67f85d7b975105a53a17d2b2360c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3911270
Commit-Queue: 王澳 <wangao...@bytedance.com>
Reviewed-by: Maya Lekova <msle...@chromium.org>
Reviewed-by: Igor Sheludko <ish...@chromium.org>
Reviewed-by: Marja Hölttä <ma...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83586}
---
M BUILD.bazel
M BUILD.gn
M src/builtins/builtins-definitions.h
M src/builtins/builtins-json.cc
M src/common/message-template.h
M src/compiler/types.cc
M src/diagnostics/objects-debug.cc
M src/diagnostics/objects-printer.cc
M src/flags/flag-definitions.h
M src/init/bootstrapper.cc
M src/init/heap-symbols.h
M src/json/json-parser.cc
M src/json/json-parser.h
M src/json/json-stringifier.cc
M src/objects/all-objects-inl.h
M src/objects/contexts.h
M src/objects/js-objects.cc
A src/objects/js-raw-json-inl.h
A src/objects/js-raw-json.cc
A src/objects/js-raw-json.h
A src/objects/js-raw-json.tq
M src/objects/map.cc
M src/objects/object-list-macros.h
M src/objects/objects-body-descriptors-inl.h
A test/mjsunit/harmony/json-parse-with-source.js
M test/mjsunit/json.js
M test/unittests/interpreter/bytecode_expectations/PrivateAccessorAccess.golden
M test/unittests/interpreter/bytecode_expectations/PrivateMethodAccess.golden
M test/unittests/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden
M tools/v8heapconst.py
30 files changed, 945 insertions(+), 174 deletions(-)

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Igor Sheludko (Gerrit)

unread,

Oct 12, 2022, 9:08:29 AM10/12/22

to 王澳, V8 LUCI CQ, victorgo...@chromium.org, Maya Lekova, Marja Hölttä, Pinpoint perf, Shu-yu Guo, Michael Hablich, v8-re...@googlegroups.com

View Change

1 comment:

Patchset:
- Patch Set #27:
  There's an issue with this CL. PTAL: https://bugs.chromium.org/p/chromium/issues/detail?id=1373770#c4

To view, visit change 3911270. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward