Closed
Bug 1394719
Opened 7 years ago
Closed 7 years ago
Optimize HasRTLChars usage
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(2 files)
4.55 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
4.68 KB,
patch
|
Details | Diff | Splinter Review |
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•7 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•7 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•7 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.
Assignee | ||
Comment 4•7 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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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•7 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
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•