Closed Bug 1392181 Opened 4 years ago Closed 4 years ago

Optimize nsTextFragment::SetTo() more

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(2 files)

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 on attachment 8899651 [details]
Bug 1392181 - part1: nsTextFragment::SetTo() shouldn't use CheckedInt

https://reviewboard.mozilla.org/r/170634/#review176214
Attachment #8899651 - Flags: review?(bugs) → review+
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.
Attachment #8899652 - Flags: review?(VYV03354) → review+
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!
Pushed by masayuki@d-toybox.com:
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
You need to log in before you can comment on or make changes to this bug.