Closed Bug 1431025 Opened 7 years ago Closed 7 years ago

Replace nsBidiUtils::HasRTLChars() implementation with encoding_rs

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file)

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.)
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.
(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 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 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.
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: