inspector: simplify dispatchProtocolMessage

[lemire]

This is in relation with bug #47457 where we found that under Windows, there seems to be a problem when converting from UTF-8 to UTF-16 a JSON message generated by key presses in the REPL.

Tracing it back, I found evidence that in common cases, each time you hit a key in the Node REPL, a call to dispatchProtocolMessage is made, passing an UTF-16 JSON message. The message is converted to UTF-8 into the local variable raw_message, and then we call parseMessage which converts back raw_message into UTF-16 and calls parseJSON on it.

This PR avoids the conversion from UTF-8 back into UTF-16. This should save some processing that is currently done with each key press.

It is possible that it is a Chesterton's fence and that the complexity is somehow necessary. Nevertheless, if this PR is correct, it would be an interesting optimization for the REPL.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/54162/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/54234/

[anonrig]

cc @nodejs/inspector @nodejs/cpp-reviewers

[tniessen]

Nit: the commit message does not follow our guidelines. Please change it to begin with an imperative verb after the subsystem prefix.

[eugeneo]

I think at this point we can confidently say there will never be a binary message in inspector protocol 😀 (it is pretty much set in stone at this point).

Why not just delete protocol::StringUtil::parseMessage? Note it is generated so needs to be deleted in the template.

[eugeneo]

PR message seems misleading. Proposed change does not seem to touch any UTF8/UTF16 shenanigans unless I am missing something.

[lemire]

Nit: the commit message does not follow our guidelines. Please change it to begin with an imperative verb after the subsystem prefix.

I changed the commit and the PR name.

[lemire]

@eugeneo

Proposed change does not seem to touch any UTF8/UTF16 shenanigans unless I am missing something.

It does. And that's a key point. We are saving some non-trivial processing.

The current code calls StringUtil::parseMessage which calls StringUtil::ParseJSON(const std::string_view) which is like so...

std::unique_ptr<Value> parseJSON(const std::string_view string) {
  if (string.empty())
    return nullptr;
  size_t expected_utf16_length =
      simdutf::utf16_length_from_utf8(string.data(), string.length());
  MaybeStackBuffer<char16_t> buffer(expected_utf16_length);
  // simdutf::convert_utf8_to_utf16 returns zero in case of error.
  size_t utf16_length = simdutf::convert_utf8_to_utf16(
      string.data(), string.length(), buffer.out());
  // We have that utf16_length == expected_utf16_length if and only
  // if the input was a valid UTF-8 string.
  if (utf16_length == 0) return nullptr;  // We had an invalid UTF-8 input.
  CHECK_EQ(expected_utf16_length, utf16_length);
  return parseJSONCharacters(reinterpret_cast<const uint16_t*>(buffer.out()),
                             utf16_length);
}

Observe how it converts to UTF-16.

Instead, we call directly StringUtil::parseJSON(v8_inspector::StringView string)...

std::unique_ptr<Value> parseJSON(v8_inspector::StringView string) {
  if (string.length() == 0)
    return nullptr;
  if (string.is8Bit())
    return parseJSONCharacters(string.characters8(), string.length());
  return parseJSONCharacters(string.characters16(), string.length());
}

See how we are bypassing simdutf::convert_utf8_to_utf16?

We are possibly skipping some allocation and some transcoding...

It is not going to make a huge difference unless you are typing really fast on your keyboard, but... code simplification is always a positive, I would think.

[lemire]

Why not just delete protocol::StringUtil::parseMessage?

Yes. Let me try.

[lemire]

@eugeneo Last commit removes parseMessage entirely since it is seemingly not needed.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/54265/

[lemire]

@addaleax @alexkozy Can you help?

This PR get the following issue under macOS in CI:

=== release test-inspector-wait-for-connection ===
Path: parallel/test-inspector-wait-for-connection
Error: --- stderr ---
Timed out waiting for matching notification (Console output matching "after wait for debugger")

If it is indeed an issue introduced by the current PR, I would like to be able to reproduce it.

Running the script with Node 20 (public release), I get...

$ node test/parallel/test-inspector-wait-for-connection.js
[test] Connecting to a child Node process
[test] Testing /json/list
[err] Debugger listening on ws://127.0.0.1:58989/8095cee9-4dfb-4cd4-a887-3171664981a3
[err] For help, see: https://nodejs.org/en/docs/inspector
[err] 
[out] before wait for debugger
[out] 
[err] Debugger attached.
[err] 
[out] after wait for debugger
[out] 
[test] Connecting to a child Node process
[test] Testing /json/list
[out] before second wait for debugger
[out] 
[err] Debugger attached.
[err] 
[out] after second wait for debugger
[out] 
[err] Debugger ending on ws://127.0.0.1:58989/8095cee9-4dfb-4cd4-a887-3171664981a3
[err] For help, see: https://nodejs.org/en/docs/inspector
[err] 
$ echo $?
0

On my macBook, building the software, I get...

$ ./out/Debug/node test/parallel/test-inspector-wait-for-connection.js
[test] Connecting to a child Node process
[test] Testing /json/list
[err] Debugger listening on ws://127.0.0.1:58995/76517d6d-976c-435c-a8fe-b912eb92f258
[err] For help, see: https://nodejs.org/en/docs/inspector
[err] 
[out] before wait for debugger
[out] 
[err] Debugger attached.
[err] 
[out] after wait for debugger
[out] 
[out] before second wait for debugger
[out] 
[test] Connecting to a child Node process
[test] Testing /json/list
[err] Debugger attached.
[err] 
[out] after second wait for debugger
[out] 
[err] Debugger ending on ws://127.0.0.1:58995/76517d6d-976c-435c-a8fe-b912eb92f258
[err] For help, see: https://nodejs.org/en/docs/inspector
[err] 
$ echo $?
0

and

$ ./out/Release/node test/parallel/test-inspector-wait-for-connection.js[test] Connecting to a child Node process
[test] Testing /json/list
[err] Debugger listening on ws://127.0.0.1:59000/9c8cb27d-b6ec-406e-9e1e-bb1678769d48
[err] For help, see: https://nodejs.org/en/docs/inspector
[err] 
[out] before wait for debugger
[out] 
[err] Debugger attached.
[err] 
[out] after wait for debugger
[out] 
[test] Connecting to a child Node process
[test] Testing /json/list
[out] before second wait for debugger
[out] 
[err] Debugger attached.
[err] 
[out] after second wait for debugger
[out] 
[err] Debugger ending on ws://127.0.0.1:59000/9c8cb27d-b6ec-406e-9e1e-bb1678769d48
[err] For help, see: https://nodejs.org/en/docs/inspector
[err] 
$ echo $?
0

So everything is fine, as far as I can see.

This code is built with the patch from the current PR, e.g., we have

  void dispatchProtocolMessage(const StringView& message) {
    std::string raw_message = protocol::StringUtil::StringViewToUtf8(message);
    per_process::Debug(DebugCategory::INSPECTOR_SERVER,
                       "[inspector received] %s\n",
                       raw_message);
    std::unique_ptr<protocol::DictionaryValue> value =
        protocol::DictionaryValue::cast(
            protocol::StringUtil::parseJSON(message));

[lemire]

The macOS test seems to pass now. :-/

[nodejs-github-bot]

Landed in 2990390