Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipc: use vql for uint types #167407

Merged
merged 2 commits into from Dec 2, 2022
Merged

ipc: use vql for uint types #167407

merged 2 commits into from Dec 2, 2022

Conversation

connor4312
Copy link
Member

@connor4312 connor4312 commented Nov 28, 2022

On the plane I was reverse-engineering ipc.ts to implement it in Rust and see if we could have a "service mode" for the CLI that we could interact with like any other vscode process. (And maybe reuse it if we have more native processes in the future)

In doing so, I noticed that numbers in the protocol--which are used at least twice in the message header and ID--were encoded as JSON. I was curious what benefits we'd get from encoding them as variable-length integers instead.

It makes the message shorter, as expected. Encode/decode time are very, very slightly lower. I'm not sure it's worth the extra complexity, but I have included it here for your consideration.

Encode speed

old length 12
old messages/s 1204819
new length 9
new messages/s 1219512
const encode = (serialize: any) => {
	const writer = new BufferWriter();
	serialize(writer, [201, 1234]);
	serialize(writer, undefined);
	return writer.buffer.byteLength;
};

const n = 10_000;
for (let i = 0; i < n * 5; i++) { // warmup
	encode(serialize);
	encode(serializeOld);
}

const a = performance.now();
for (let i = 0; i < n; i++) {
	encode(serializeOld);
}
const b = performance.now();
for (let i = 0; i < n; i++) {
	encode(serialize);
}
const c = performance.now();

Decode speed

old messages/s 793651
new messages/s 806452
const newData = VSBuffer.fromByteArray([4, 2, 6, 201, 1, 6, 210, 9, 0]);
const oldData = VSBuffer.fromByteArray([4, 0, 0, 0, 2, 6, 201, 1, 6, 210, 9, 0]);

const n = 10_000;
for (let i = 0; i < n * 5; i++) { // warmup
	deserialize(new BufferReader(newData));
	deserializeOld(new BufferReader(oldData));
}
const a = performance.now();
for (let i = 0; i < n; i++) {
	deserialize(new BufferReader(newData));
}
const b = performance.now();
for (let i = 0; i < n; i++) {
	deserializeOld(new BufferReader(oldData));
}
const c = performance.now();

On the plane I was reverse-engineering ipc.ts to implement it in Rust
and see if we could have a "service mode" for the CLI that we could
interact with like any other vscode process.

In doing so, I noticed that numbers in the protocol--which are used at
least twice in the message header and ID--were encoded as JSON. I was
curious what benefits we'd get from encoding them as variable-length
integers instead.

It makes the message shorter, as expected. Encode/decode time are very,
very slightly lower. I'm not sure it's worth the extra complexity, but
I have included it here for your consideration.
@connor4312
Copy link
Member Author

hm, I had unit tests passing locally 😛 if there's interest I'll circle back and fix those up

@alexdima
Copy link
Member

👍 to doing this from my side. This particular byte-protocol is owned by @joaomoreno

If you'd like to give it a try and optimize it too, there's a different one for the extension host here.

@alexdima alexdima requested review from joaomoreno and removed request for alexdima November 28, 2022 16:55
@connor4312
Copy link
Member Author

Huh, I didn't realize that, I always assumed ipc.ts was the underlying transport. Do you recall why we have a different implementation there vs generic ipc?

@alexdima
Copy link
Member

I don't quite remember, we kept refactoring things. I think they might have ended up in a situation where rpcProtocol.ts is built on top of ipc.ts.

joaomoreno
joaomoreno previously approved these changes Nov 30, 2022
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@connor4312 connor4312 merged commit 0899758 into main Dec 2, 2022
@connor4312 connor4312 deleted the connor4312/vql-ipc branch December 2, 2022 23:54
jrieken added a commit that referenced this pull request Jan 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants