Closed
Bug 1392181
Opened 7 years ago
Closed 7 years ago
Optimize nsTextFragment::SetTo() more
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
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.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
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 6•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
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
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f77110e64bbd
https://hg.mozilla.org/mozilla-central/rev/08c538a18540
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•