Skip to content

util: add fast path for Latin1 decoding #55275

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.

Sign up for GitHub

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

Merged
nodejs-github-bot merged 15 commits into from
Dec 3, 2024
Merged

util: add fast path for Latin1 decoding #55275

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b12ba58
util: add fast path for Latin1 decoding
Oct 5, 2024
3c239bd
handle input correctly in decode functions
Oct 5, 2024
17f317e
repair
Oct 5, 2024
e0a40c6
Update lib/internal/encoding.js
mertcanaltin Oct 5, 2024
b7420b5
add ignoreBOM and fatal support to decodeLatin1
Oct 5, 2024
8bc747e
lint
Oct 5, 2024
fced029
update tests to support ignoreBOM and fatal for decodeLatin1
Oct 5, 2024
af7cca5
lint
Oct 5, 2024
659f1b6
correct BOM check in decodeLatin1 for uint8_t comparison
Oct 5, 2024
b92e571
add BOM test case to cover 0xFF in decodeLatin1
Oct 5, 2024
94109bf
Update encoding_binding.cc
mertcanaltin Oct 6, 2024
6dec37f
delete result.resize
Oct 6, 2024
f62f4c4
Update encoding_binding.cc
mertcanaltin Oct 7, 2024
f96e2ef
Update encoding_binding.cc
mertcanaltin Oct 7, 2024
77bc5ee
update text-decoder to use windows-1252 encoding
Oct 8, 2024
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
Prev Previous commit
Next Next commit
Update lib/internal/encoding.js 
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
  • Loading branch information
@mertcanaltin @anonrig
mertcanaltin and anonrig authored Oct 5, 2024
commit e0a40c6b9bdf335097b00a7c007949cd660557b3
2 changes: 1 addition & 1 deletion lib/internal/encoding.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ function makeTextDecoderICU() {
this[kLatin1FastPath] = enc === 'windows-1252';

@ChALkeR ChALkeR Nov 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line specifically uses the wrong assumption that Latin1 and windows-1252 are the same encoding

They aren't

@mertcanaltin mertcanaltin Nov 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm catching now in MDN api docs, say this latin1 same windows-1252: https://developer.mozilla.org/en-US/docs/Web/API/Encoding_API/Encodings

@mertcanaltin mertcanaltin Nov 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was following the WHATWG Encoding Standard, which
explicitly states that in web-compatible software, latin1, iso-8859-1, and ascii
are all just labels for windows-1252

from the spec: "these are synonyms: latin1 and ascii are just labels for
windows-1252, and any software following this standard will, for example, decode 0x80
as U+20AC (€)

however, my implementation was still flawed - it didn't properly handle the 0x80-0x9F
range mapping. The fast path needs a lookup table for those bytes to correctly map them
to their windows-1252 code points (€, smart quotes, etc)

so while the WHATWG standard does treat them as equivalent, my implementation failed to
properly decode them. I support the revert, and if there's interest in a performance
optimization here, it would need to properly implement the windows-1252 mapping table

@ChALkeR ChALkeR Nov 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

WHATWG mapping Latin1 to windows-1252 doesn't mean that Latin1 and windows-1252 are equivalent.

windows-1252 decoder is compatible with decoding Latin1 bytes, this is why the standard can use that mapping, the same way it can alias ascii to windows-1252

There is no Latin1 (or ascii) in TextDecoder, new TextDecoder('Latin1') creates a windows-1252 decoder which is capable of decoding Latin1, as it's a subset of windows-1252.

new TextDecoder('ascii') also returns a windows-1252 decoder by spec, which is reasonable as ascii is also a subset of windows-1252 and that decoder can well decode ascii bytes.

But we can't implement windows-1252 with Latin1 (or with ascii-only).

We can't even make new TextDecoder('Latin1') / new TextDecoder('ascii') return a Latin1-only (or ascii-only) decoder, as that would break compat.

It should fully support windows-1252

See also our own doc:

node/doc/api/buffer.md

Lines 229 to 234 in 7643c2a

Modern Web browsers follow the [WHATWG Encoding Standard][] which aliases
both `'latin1'` and `'ISO-8859-1'` to `'win-1252'`. This means that while doing
something like `http.get()`, if the returned charset is one of those listed in
the WHATWG specification it is possible that the server actually returned
`'win-1252'`-encoded data, and using `'latin1'` encoding may incorrectly decode
the characters.

@mertcanaltin mertcanaltin Nov 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for detail!

this[kHandle] = undefined;

if (!this[kUTF8FastPath]) {
if (!this[kUTF8FastPath] && !this[kLatin1FastPath]) {
this.#prepareConverter();
Comment thread
anonrig marked this conversation as resolved.
}
}
Expand Down