feat: use TextEncoder and TextDecoder for utf8 strings
#4513
Conversation
master, af1a9c9 seia-soto:textencoder, 1de005e |
packages/adblocker/src/data-view.ts
Outdated
| this.buffer[this.pos++] = str.charCodeAt(i); | ||
| } | ||
| const { written } = TEXT_ENCODER.encodeInto(raw, this.buffer.subarray(this.pos + 4)); | ||
| this.pushUint32(written); |
There was a problem hiding this comment.
Isn't that wasteful in terms of final engine size?
There was a problem hiding this comment.
encodeInto doesn't emit C-style NULL character which indicates the end of string at the end. We can consume the string dynamically when reading the binary but I think it's simpler to have a final length of string at the front.
There was a problem hiding this comment.
My comment was about using 32-bits instead of potentially 8-bits or 16-bits for smaller strings.
There was a problem hiding this comment.
I think we can change this to 16 bits (= ~65535 chars). 👍
There was a problem hiding this comment.
Reduced in d6032eb (#4513)
There was a problem hiding this comment.
I think they're barely called which is not my expect. I only got 5 logs:
17
65
989
117
46
93
There was a problem hiding this comment.
Preferring to use getLength and pushLength as pushUint16 is breaking the test.
There was a problem hiding this comment.
I think they're barely called which is not my expect. I only got 5 logs:
17 65 989 117 46 93
@seia-soto Did you figure out why they are barely called and if it's expected? Because if this method is almost not called maybe it's preferable to keep a simpler code for the variable length part.
There was a problem hiding this comment.
@remusao I got all pushUTF8 calls and these are the entries:
- Tracker DB properties
- Cosmetic selectors with unicode
- Use of
optionValuein Network filters (csp, redirect, and replace) - Resource content (including scriptlets)
- Preprocessor conditions
I don't think we will see those frequently in testing or benchmarking setup.
There was a problem hiding this comment.
I don't think we will see those frequently in testing or benchmarking setup.
We need to check with production assets being loaded. I don't think you will learn anything otherwise. With which lists did you do the measurement above?
The point being: if when loading full assets with all lists/resources/etc. this method is not called a lot, then I think it's best to keep the simpler version without the variadic length logic (that only makes sense if it has a measurable impact on final binary size)
- ~65535 ASCII only characters
> 147.50420889870574 / 156.1296767089117 // benchEngineDeserialization
0.944754463135876
> 147.50420889870574 / 148.7394726802865 // benchEngineSerialization
0.9916951179177841seia-soto:textencoder, 65764e9
master, de7bfb5
|
chrmod
added
PR: Bug Fix 🐛
|
This PR is awaiting for the final review. Further changes are expected to be categorized as a performance improvement. |
| // Restore pos to push length | ||
| this.setPos(pos); | ||
| this.pushLength(written); | ||
| // Reflect written bytes to pos | ||
| this.setPos(this.pos + written); |
There was a problem hiding this comment.
| // Restore pos to push length | |
| this.setPos(pos); | |
| this.pushLength(written); | |
| // Reflect written bytes to pos | |
| this.setPos(this.pos + written); | |
| this.pushLength(written); | |
| this.setPos(pos + written); |
There was a problem hiding this comment.
We can't do that here. Despite pushLength is located later than encodeInto, we still want the length byte to be earlier than the actual payload. In that case, we're writing the length after the payload. Therefore, we need to recall the pos before we wrote the payload.
| @@ -20,6 +20,9 @@ export const EMPTY_UINT32_ARRAY = new Uint32Array(0); | |||
| // Check if current architecture is little endian | |||
| const LITTLE_ENDIAN: boolean = new Int8Array(new Int16Array([1]).buffer)[0] === 1; | |||
|
|
|||
| // TextEncoder doesn't need to be recreated every time unlike TextDecoder | |||
| const TEXT_ENCODER = new TextEncoder(); | |||
There was a problem hiding this comment.
alternatively you could do:
const encoder = new TextEncoder();
const encode = encoder.encode.bind(encoder);
Co-authored-by: Krzysztof Modras <[email protected]>
Co-authored-by: Krzysztof Modras <[email protected]>
fixes #4424
This PR replaces punycode encoder and decoder with
TextEncoderandTextDecoderfor utf8 strings.\ufeffshould be skipped when decoding to ensure the original formUint8Array.subarraydoesn't copy the array but provides a direct interface to subarrayTextEncoder.encodeIntodoesn't produce EOL character ( NULL, U+0000 ) but we don't care becauseTextDecodercan stop nicely when provided buffer endsTo be safe: