1431025 - Replace nsBidiUtils::HasRTLChars() implementation with encoding_rs

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[Henri Sivonen (:hsivonen)]

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.)

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1431025 - Use encoding_rs::mem::is_utf16_bidi() as the implementation of HasRTLChars().

Review commit: https://reviewboard.mozilla.org/r/213924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213924/

Comment 2

[Henri Sivonen (:hsivonen)]

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.

Comment 3

[Henri Sivonen (:hsivonen)]

(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.

Comment 4

[Jonathan Kew [: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

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 8943578 [details]
Bug 1431025 - Use encoding_rs::mem::is_utf16_bidi() as the implementation of HasRTLChars().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213924/diff/1-2/

Comment 6

[Henri Sivonen (:hsivonen)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a92be6b431a
Use encoding_rs::mem::is_utf16_bidi() as the implementation of HasRTLChars(). r=jfkthame

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9a92be6b431a