2061103 - Update encoding_rs to main before 0.8.40

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Henri Sivonen (:hsivonen)]

encoding_rs 0.8.40 will

• Fix a buffer boundary bug with two-byte legacy decoders.
• Fix a bug when simd-accel isn't used. (Not relevant to Mozilla builds but, I believe, relevant to some downstream builds.)
• Improve performance.

Let's get it reviewed here before publishing to crates.io.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 2061103 - Update encoding_rs to main before 0.8.40.

Comment 2

[Henri Sivonen (:hsivonen)]

https://perf.compare/compare-results?baseRev=b37a40c484fe00353281ddd704b140ff936fae78&newRev=1c2eaad26519c42c4c2e80c11763f57b89d38ea6&baseRepo=try&newRepo=try&framework=13

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=b37a40c484fe00353281ddd704b140ff936fae78
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=1c2eaad26519c42c4c2e80c11763f57b89d38ea6

Comment 3

[Henri Sivonen (:hsivonen)]

Examples of notable perf wins

Note: "Decode" is HTML, other cases are plain text

Note: These are with SIMD (and for x86_64 multiversioning) enabled at opt_level 3. Making sure that these work in the Firefox build context is TODO.

M3 Pro

Decode Russian UTF-8 to UTF-8: 3.99x
Decode Vietnamese UTF-8 to UTF-8: 3.10x

Ensure UTF-16 validity: 1.75x

Decode French windows-1252 to UTF-16: 1.20x

Convert Latin1 to UTF-8: 1.42x

Raspberry Pi 4 in 32-bit mode

Decode ASCII as windows-1252 to UTF-8: 3.13x
Decode ASCII as windows-1252 to UTF-16: 2.07x
Decode ASCII as UTF-8 to UTF-16: 2.02x

Decode German UTF-8 to UTF-16: 1.76x
Decode Czech windows-1250 to UTF-16: 1.78x

Ensure UTF-16 validity: 2.21x

Convert Latin1 to UTF-8: 1.87x

Zen 3

Decode Hebrew UTF-8 to UTF-8: 7.45x
Decode Arabic UTF-8 to UTF-8: 7.10x
Decode Vietnamese UTF-8 to UTF-8: 6.19x
Decode Greek UTF-8 to UTF-8: 6.06x

Encode French from UTF-16 to UTF-8: 2.15x

Convert Latin1 to UTF-16: 1.80x

Ensure UTF-16 validity: 1.70x

Convert ASCII UTF-8 to UTF-16: 1.63x

Decode French UTF-8 to UTF-16: 1.49x
Decode Vietnamese UTF-8 to UTF-16: 1.48x

Skylake

Decode Czech UTF-8 to UTF-16: 1.79x
Decode Vietnamese UTF-8 to UTF-16: 1.56x

Decode Hebrew UTF-8 to UTF-8: 6.36x
Decode Arabic UTF-8 to UTF-8: 5.85x

Convert Latin1 to UTF-16: 1.22x
Convert UTF-16 to Latin1: 1.17x

Ensure UTF-16 validity: 2.32x

Comment 4

[Henri Sivonen (:hsivonen)]

https://perf.compare/compare-results?baseRev=3c21bf69fa0927662a211ed41a9d70544c9a6f7a&newRev=15980ec461a4426d5e606eaa0b821dc81fa25146&baseRepo=try&newRepo=try&framework=13

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=3c21bf69fa0927662a211ed41a9d70544c9a6f7a
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=15980ec461a4426d5e606eaa0b821dc81fa25146

Comment 5

[Henri Sivonen (:hsivonen)]

Attempting to override opt-level of encoding_rs to 3.

https://perf.compare/compare-results?baseRev=3c21bf69fa0927662a211ed41a9d70544c9a6f7a&newRev=5ee4ecf0f2587feefdd2ab302f8ed07d35a241d0&baseRepo=try&newRepo=try&framework=13

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=3c21bf69fa0927662a211ed41a9d70544c9a6f7a
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=5ee4ecf0f2587feefdd2ab302f8ed07d35a241d0

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #5)

Attempting to override opt-level of encoding_rs to 3.

Not sure if this config does what I meant, but at least it looks like the tweak from comment 5 isn't worth doing:
https://perf.compare/compare-results?baseRev=15980ec461a4426d5e606eaa0b821dc81fa25146&baseRepo=try&newRev=5ee4ecf0f2587feefdd2ab302f8ed07d35a241d0&newRepo=try&framework=6&test_version=mann-whitney-u&search=Strings+Perf&filter_status=improvement%2Cregression

However, this looks concerning with the on-Phabricator patch and run from comment 4:
https://perf.compare/compare-results?baseRev=3c21bf69fa0927662a211ed41a9d70544c9a6f7a&newRev=15980ec461a4426d5e606eaa0b821dc81fa25146&baseRepo=try&newRepo=try&framework=6&search=PerfLatin1toUTF16+Thousand

Per comment 3, it should be showing a bigger improvement.

Comment 7

[Henri Sivonen (:hsivonen)]

Let's try building all Rust crates at opt_level 3 to make sure that issues of setting precedence don't cause encoding_rs to get built at opt_level 2 anyway.

https://perf.compare/compare-results?baseRev=8d2d2bc4e508c921306ae497cd2a940b273da4d1&newRev=9c23870caacba2c3d7fef359a74947a605ec40c0&baseRepo=try&newRepo=try&framework=13

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=8d2d2bc4e508c921306ae497cd2a940b273da4d1
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=9c23870caacba2c3d7fef359a74947a605ec40c0

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/db95f34392f6
https://hg.mozilla.org/integration/autoland/rev/947ae4c963b9
Update encoding_rs to main before 0.8.40. r=supply-chain-reviewers,afranchuk,dminor

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/90ce18e62135
https://hg.mozilla.org/integration/autoland/rev/84f572ba4089
Revert "Bug 2061103 - Update encoding_rs to main before 0.8.40. r=supply-chain-reviewers,afranchuk,dminor" for causing encoding related wpt failures

Comment 10

[Norisz Fay [:noriszfay]]

Backed out for causing encoding related wpt failures

Backout link

Push with failures

Failure log
Failure log 2

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f5b3e3f51b2a
https://hg.mozilla.org/integration/autoland/rev/eb5fc34815c6
Update encoding_rs to main before 0.8.40. r=supply-chain-reviewers,afranchuk,dminor

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Norisz Fay [:noriszfay] from comment #10)

Backed out for causing encoding related wpt failures

Relanded with the test expectations adjusted to make the newly-passed cases expected.

Comment 13

[Henri Sivonen (:hsivonen)]

It turns out that the UTF-16 to UTF-8 and UTF-16 to Latin1 numbers in comment 3 are not applicable to Gecko; see bug 2039417 comment 1.

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/eb5fc34815c6

Comment 15

[Henri Sivonen (:hsivonen)]

Note for sheriffing: This change added function multiversioning on x86 and x86_64, so a binary size increase on those CPU architectures is expected.

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Bug 2061103 - Update encoding_rs to main before 0.8.40.

Original Revision: https://phabricator.services.mozilla.com/D316770

Comment 17

[Phabricator Automation]

firefox-esr153 Uplift Approval Request

• User impact if declined/Reason for urgency: The older version of encoding_rs has a very bad bug (https://github.com/hsivonen/encoding_rs/issues/125) that can manifest in a configuration that isn't used in Mozilla-shipped builds of Firefox but may be used by some Linux distros, etc. Strangely, despite the badness, the issue has never been reported in Firefox context. Still, it would be prudent to fix it on ESR.

(Not requesting uplift to ESR 140 due to near EOL and MSRV issues. Not requesting uplift to ESR 115 due to it being for Mozilla-shipped builds. Also MSRV.)

(If this uplift is accepted, it makes sense to uplift https://phabricator.services.mozilla.com/D324016 also.)

• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Despite the changeset being huge, the code has been on Nightly for a couple of weeks without incident.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 18

[Henri Sivonen (:hsivonen)]

Fix verified in Nightly?: yes

That's slightly incorrect. The fix that the uplift is about has been manually verified in the code that is in the changeset but not in the context of Nightly, since Nightly doesn't ship with that config and we don't have steps to repro in the context of a full Firefox build.

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/d7013223ee48