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

perf(web): optimize single pass utf8 decoding #16593

Merged
merged 6 commits into from
Nov 11, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 38 additions & 32 deletions ext/web/08_text_encoding.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
#fatal;
/** @type {boolean} */
#ignoreBOM;
/** @type {boolean} */
#utf8SinglePass;

/** @type {number | null} */
#rid = null;
Expand All @@ -56,6 +58,7 @@
this.#encoding = encoding;
this.#fatal = options.fatal;
this.#ignoreBOM = options.ignoreBOM;
this.#utf8SinglePass = encoding === "utf-8" && !options.fatal;
this[webidl.brand] = webidl.brand;
}

Expand All @@ -81,7 +84,7 @@
* @param {BufferSource} [input]
* @param {TextDecodeOptions} options
*/
decode(input = new Uint8Array(), options = {}) {
decode(input = new Uint8Array(), options = undefined) {
webidl.assertBranded(this, TextDecoderPrototype);
const prefix = "Failed to execute 'decode' on 'TextDecoder'";
if (input !== undefined) {
Expand All @@ -91,40 +94,43 @@
allowShared: true,
});
}
options = webidl.converters.TextDecodeOptions(options, {
prefix,
context: "Argument 2",
});
let stream = false;
if (options !== undefined) {
options = webidl.converters.TextDecodeOptions(options, {
prefix,
context: "Argument 2",
});
stream = options.stream;
}
Comment on lines +97 to +104
Copy link
Member Author

Choose a reason for hiding this comment

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

We should come up with a general improvement in webidl for cases like this. I think the optimal way would require new Function codegen.

Copy link

@ghost ghost Nov 11, 2022

Choose a reason for hiding this comment

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

For what it's worth, Evan (Discord) had been saying that for over a year. The entire WebIDL bindings setup should be via JiT-compiled codegen.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah :(

never too late though


try {
try {
if (ArrayBufferIsView(input)) {
input = new Uint8Array(
input.buffer,
input.byteOffset,
input.byteLength,
);
} else {
input = new Uint8Array(input);
}
} catch {
// If the buffer is detached, just create a new empty Uint8Array.
// Note from spec: implementations are strongly encouraged to use an implementation strategy that avoids this copy.
// When doing so they will have to make sure that changes to input do not affect future calls to decode().
if (input.byteLength === 0) {
littledivy marked this conversation as resolved.
Show resolved Hide resolved
// This handles the case where buffer is detached too.
input = new Uint8Array();
}
if (
ObjectPrototypeIsPrototypeOf(
SharedArrayBuffer.prototype,
input.buffer,
)
) {
// We clone the data into a non-shared ArrayBuffer so we can pass it
// to Rust.
// `input` is now a Uint8Array, and calling the TypedArray constructor
// with a TypedArray argument copies the data.
input = new Uint8Array(input);
}

if (!options.stream && this.#rid === null) {
// if (
// ObjectPrototypeIsPrototypeOf(
// SharedArrayBuffer.prototype,
// input.buffer,
// )
// ) {
// // We clone the data into a non-shared ArrayBuffer so we can pass it
// // to Rust.
// // `input` is now a Uint8Array, and calling the TypedArray constructor
// // with a TypedArray argument copies the data.
// input = new Uint8Array(input);
// }
littledivy marked this conversation as resolved.
Show resolved Hide resolved

// Fast path for single pass encoding.
if (!stream && this.#rid === null) {
// Fast path for utf8 single pass encoding.
if (this.#utf8SinglePass) {
return ops.op_encoding_decode_utf8(input, this.#ignoreBOM);
}

return ops.op_encoding_decode_single(
input,
this.#encoding,
Expand All @@ -140,9 +146,9 @@
this.#ignoreBOM,
);
}
return ops.op_encoding_decode(input, this.#rid, options.stream);
return ops.op_encoding_decode(input, this.#rid, stream);
} finally {
if (!options.stream && this.#rid !== null) {
if (!stream && this.#rid !== null) {
core.close(this.#rid);
this.#rid = null;
}
Expand Down
34 changes: 34 additions & 0 deletions ext/web/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ pub fn init<P: TimersPermission + 'static>(
op_base64_btoa::decl(),
op_encoding_normalize_label::decl(),
op_encoding_decode_single::decl(),
op_encoding_decode_utf8::decl(),
op_encoding_new_decoder::decl(),
op_encoding_decode::decl(),
op_encoding_encode_into::decl(),
Expand Down Expand Up @@ -179,6 +180,39 @@ fn op_encoding_normalize_label(label: String) -> Result<String, AnyError> {
Ok(encoding.name().to_lowercase())
}

#[op(v8)]
fn op_encoding_decode_utf8<'a>(
scope: &mut v8::HandleScope<'a>,
zero_copy: &[u8],
ignore_bom: bool,
) -> Result<serde_v8::Value<'a>, AnyError> {
let buf = &zero_copy;

let buf = if ignore_bom
&& buf.len() >= 3
&& buf[0] == 0xef
&& buf[1] == 0xbb
&& buf[2] == 0xbf
{
&buf[3..]
} else {
buf
};

// If `String::new_from_utf8()` returns `None`, this means that the
// length of the decoded string would be longer than what V8 can
// handle. In this case we return `RangeError`.
//
// For more details see:
// - https://encoding.spec.whatwg.org/#dom-textdecoder-decode
// - https://github.com/denoland/deno/issues/6649
// - https://github.com/v8/v8/blob/d68fb4733e39525f9ff0a9222107c02c28096e2a/include/v8.h#L3277-L3278
match v8::String::new_from_utf8(scope, buf, v8::NewStringType::Normal) {
littledivy marked this conversation as resolved.
Show resolved Hide resolved
littledivy marked this conversation as resolved.
Show resolved Hide resolved
Some(text) => Ok(serde_v8::from_v8(scope, text.into())?),
None => Err(range_error("string too long")),
}
}

#[op]
fn op_encoding_decode_single(
data: &[u8],
Expand Down