Optimize HasRTLChars usage

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/dom/base/nsGenericDOMDataNode.cpp#337-359
is super inefficient.
We end up copying data currently twice, since TextFragment copies too and we also do RTL scanning for the whole text even though it should be enough to do it only for the new text.
(Assignee)

Comment 1

2 years ago
When typing in middle of some <input> or <textarea> better to copy memory less and also RTL detection can be faster.
Depends on bug 1393232 
 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9786b025f6338a5d009805cf4b3c2ebd55b40cb6
Attachment #8902225 - Flags: review?(ehsan)

Comment 2

2 years ago
Comment on attachment 8902225 [details] [diff] [review]
insert_text_to_text_node.diff

Review of attachment 8902225 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGenericDOMDataNode.cpp
@@ +342,3 @@
>      // Allocate new buffer
>      int32_t newLength = textLength - aCount + aLength;
> +    nsString to;

Any reason not to use an nsAutoString here?  It may save on some allocations for small strings...

@@ +365,2 @@
>      bool ok =
> +      mText.SetTo(to, false, use2b);

Nit: one line please.
Attachment #8902225 - Flags: review?(ehsan) → review+
(Assignee)

Comment 3

2 years ago
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #2)
> 
> Any reason not to use an nsAutoString here?  It may save on some allocations
> for small strings...
Because I want string buffer here to avoid copying then in nsTextFragment. I'll add a comment.

> 
> @@ +365,2 @@
> >      bool ok =
> > +      mText.SetTo(to, false, use2b);
> 
> Nit: one line please.

Comment 5

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9931b3249b03
Optimize HasRTLChars usage and reduce malloc/free, r=ehsan

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9931b3249b03
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 7

2 years ago
Could this patch cause the following crashes?
https://crash-stats.mozilla.com/report/index/f5b7ce8c-c508-441d-9393-cbe890170903
https://crash-stats.mozilla.com/report/index/e7d157b8-4348-446f-8298-ebeb10170902

This started right after this bug has landed, and the crash happens after clicking the prompt to update Nightly. Even with Nightly crashing, Nightly still starts with the updated version.
Using Windows 10 x86, latest Nightly, Hebrew locale.
(Assignee)

Comment 8

2 years ago
I don't see anything in the stack traces hinting that this could have caused those crashes. 
Could you please file a new bug, Core: Networking Cache, I think
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.