1392181 - Optimize nsTextFragment::SetTo() more

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

After landing the patches for bug 1391538, I still see following 2 issues in profile on my machine (https://perfht.ml/2wmO18s):

1:
> mozilla::EditorBase::SetTextImpl(mozilla::dom::Selection &,nsAString const &,mozilla::dom::Text &)
> nsGenericDOMDataNode::SetData(nsAString const &)
> nsGenericDOMDataNode::SetTextInternal(unsigned int,unsigned int,char16_t const *,unsigned int,bool,CharacterDataChangeInfo::Details *)
> nsTextFragment::SetTo(char16_t const *,int,bool,bool)
> mozilla::CheckedInt<unsigned int>::operator*=<unsigned __int64>(unsigned __int64)

CheckedInt's operator isn't inline method. That might cause unnecessary cost. Additionally, using CheckedInt is wrong since nsTextFrament manages text length with mState.mLength which is uint32_t but only 29 bits.

2:
> nsGenericDOMDataNode::SetTextInternal(unsigned int,unsigned int,char16_t const *,unsigned int,bool,CharacterDataChangeInfo::Details *)
> nsTextFragment::SetTo(char16_t const *,int,bool,bool)
> nsTextFragment::UpdateBidiFlag(char16_t const *,unsigned int)
> HasRTLChars(char16_t const *,unsigned int)

If mState.mIs2b, HasRTLChars() runs every time even if it has only ASCII characters. So, we need to optimize it.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1905eb39e13da2fd569abd726718e1b5c7709607

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4409f1d1431cfd146a4ff82529a798be252f8a2

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1392181 - part1: nsTextFragment::SetTo() shouldn't use CheckedInt

nsTextFragment::SetTo() may be hot path of performance test but it needs to be
careful for computing allocation size.  However, nsTextFragment uses uint32_t
with only 29 bits to store its text length and using CheckedInt sometimes causes
appearing CheckedInt in profile.  So, using CheckedInt in it doesn't make sense
(and also wrong).

This patch defines NS_MAX_TEXT_FRAGMENT_LENGTH and make SetTo() check the length
of new text by itself.

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1392181 - part2: HasRTLChars() should check if the character is at least equal or larger than the minimum RTL character, U+0590

HasRTLChars() appears in profile of attachment 8848015 [details] after landing the patches
for bug 1391538 because the landing made text in <input> or <textarea> always
treated as non-single byte characters.  Therefore, HasRTLChars() is now called
by nsTextFragment::SetTo() a lot in editors.

HasRTLChar() checks if it's in an RTL character for each character until it
becomes true.  However, if character is less than the minimum RTL character,
U+0590, it works faster at least for Western languages.

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8899651 [details]
Bug 1392181 - part1: nsTextFragment::SetTo() shouldn't use CheckedInt

https://reviewboard.mozilla.org/r/170634/#review176214

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8899652 [details]
Bug 1392181 - part2: HasRTLChars() should check if the character is at least equal or larger than the minimum RTL character, U+0590

https://reviewboard.mozilla.org/r/170636/#review176238

::: intl/unicharutil/util/nsBidiUtils.h:261
(Diff revision 1)
>   *  according to
>   *  http://unicode.org/Public/UNIDATA/extracted/DerivedBidiClass.txt and
>   *  http://www.unicode.org/roadmaps/
>   */
>  
> +#define MIN_RTL_CHAR (0x0590)

Is this needed in a public header? nsBidiUtils.cpp is the only user.

Also, please use |const uint32_t kMinRTLChar| in the mozilla namespace rather than a macro.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ca06a7efab81eb9466902b67d6a1089edf54f7

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8899651 [details]
Bug 1392181 - part1: nsTextFragment::SetTo() shouldn't use CheckedInt

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

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8899652 [details]
Bug 1392181 - part2: HasRTLChars() should check if the character is at least equal or larger than the minimum RTL character, U+0590

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

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8899652 [details]
Bug 1392181 - part2: HasRTLChars() should check if the character is at least equal or larger than the minimum RTL character, U+0590

https://reviewboard.mozilla.org/r/170636/#review176238

> Is this needed in a public header? nsBidiUtils.cpp is the only user.
> 
> Also, please use |const uint32_t kMinRTLChar| in the mozilla namespace rather than a macro.

Thank you!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f77110e64bbd
part1: nsTextFragment::SetTo() shouldn't use CheckedInt r=smaug
https://hg.mozilla.org/integration/autoland/rev/08c538a18540
part2: HasRTLChars() should check if the character is at least equal or larger than the minimum RTL character, U+0590 r=emk

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f77110e64bbd
https://hg.mozilla.org/mozilla-central/rev/08c538a18540