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())
returnnullptr;
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) returnnullptr; // We had an invalid UTF-8 input.CHECK_EQ(expected_utf16_length, utf16_length);
returnparseJSONCharacters(reinterpret_cast<constuint16_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)
returnnullptr;
if (string.is8Bit())
returnparseJSONCharacters(string.characters8(), string.length());
returnparseJSONCharacters(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

voiddispatchProtocolMessage(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