Replace nsBidiUtils::HasRTLChars() implementation with encoding_rs

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
I have a Rust SIMD implementation of UTF-16 bidiness check that on a Haswell i7 shows the following improvement compared to a Rust ALU implementation that I believe (but haven't verified) to be faster than our current C++ ALU code:

 bench_mem_is_utf16_bidi_de_1000  261,488 (3824 MB/s)                  38,708 (25834 MB/s)                       -222,780  -85.20% 
 bench_mem_is_utf16_bidi_ja_1000  524,420 (1906 MB/s)                  125,482 (7969 MB/s)                       -398,938  -76.07% 
 bench_mem_is_utf16_bidi_ru_1000  292,927 (3413 MB/s)                  64,627 (15473 MB/s)                       -228,300  -77.94% 
 bench_mem_is_utf16_bidi_th_1000  503,164 (1987 MB/s)                  89,577 (11163 MB/s)                       -413,587  -82.20% 

We should use the fast code in Gecko.

(I don't know why Japanese shows a lesser improvement than Thai. Based on the structure of the code, they should benefit equally.)
(Assignee)

Updated

a year ago
Depends on: 1428774
(Assignee)

Updated

a year ago
Depends on: 1431356
(Assignee)

Updated

a year ago
Depends on: 1432728
(Assignee)

Comment 2

a year ago
For non-PGO, this is indeed even a bigger win compared to our C++ than compared to ALU Rust:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1d611c4897eb8b6a1115ef71b087bd93c9027620&newProject=try&newRevision=a597f0b9d49b0c665244a3caf3dbdc52ed33efeb&framework=6

Compared to PGO C++, it's not as big a win as compared to non-PGO ALU Rust, but it's still a very big win.
(Assignee)

Comment 3

a year ago
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> Compared to PGO C++, it's not as big a win as compared to non-PGO ALU Rust,
> but it's still a very big win.

For scripts below the main bidi block. For the script above the main bidi block, it looks like C++ PGO is a pessimization, so the win compared to PGO is greater than the win compared to non-PGO.
(Assignee)

Updated

a year ago
Attachment #8943578 - Flags: review?(jfkthame)
Comment on attachment 8943578 [details]
Bug 1431025 - Use encoding_rs::mem::is_utf16_bidi() as the implementation of HasRTLChars().

https://reviewboard.mozilla.org/r/213924/#review221208

::: intl/unicharutil/util/nsBidiUtils.h:162
(Diff revision 1)
>    /**
> -   * Give a 16-bit (UTF-16) text buffer and length
> +   * Give a 16-bit (UTF-16) text buffer
>     * @return true if the string contains right-to-left characters
>     */
> -   bool HasRTLChars(const char16_t* aText, uint32_t aLength);
> -
> +  inline bool HasRTLChars(mozilla::Span<const char16_t> aBuffer) {
> +    // Span ensures we never pass a nullptr to Rust--even if the 

trailing space
Attachment #8943578 - Flags: review?(jfkthame) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8943578 [details]
Bug 1431025 - Use encoding_rs::mem::is_utf16_bidi() as the implementation of HasRTLChars().

https://reviewboard.mozilla.org/r/213924/#review221208

> trailing space

Fixed. Thanks.

Comment 7

a year ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a92be6b431a
Use encoding_rs::mem::is_utf16_bidi() as the implementation of HasRTLChars(). r=jfkthame

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a92be6b431a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.