Closed
Bug 582852
Opened 14 years ago
Closed 14 years ago
Optimize large text node editing
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla2.0b5
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
1.42 KB,
patch
|
Details | Diff | Splinter Review |
We perform really badly when we're editing a large text node (which could be common with bug 240933). We're spending most of our time figuring out if the text for the node is ascii, in which case we spend even more time trying to convert wchar_t's into char's. I have a simple patch which should fix this situation. Basically, for any text node larger than 10KB, I'm simply assuming that it's a wide char string, and I'm treating it as such. This has a memory cost, for sure, but it will make text editing operations so much more efficient. As large text nodes are not common on the web, I'm positive that the memory cost here is acceptable, and is worth paying the cost for in order to get fast editing experience when dealing with large textareas.
Assignee | ||
Updated•14 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #461134 -
Flags: review?(jst)
Comment 2•14 years ago
|
||
I thought we also had layout optimizations for "ascii" textframes. Is that no longer the case?
Comment 3•14 years ago
|
||
General note: we can get some of the benefits of this by just having a sane insert api on textfragments.
Assignee | ||
Comment 4•14 years ago
|
||
Boris, given our discussion on IRC, do you still think we should take an insert API on text fragments instead of this patch? In response to comment 2, roc believed that we do have such optimizations, but I'm not really sure what they are.
Comment 5•14 years ago
|
||
> do you still think we should take an insert API on text fragments instead of > this patch? I think it would be safer in terms of not disturbing other parts of the system. > but I'm not really sure what they are. Search for TEXT_IS_8BIT (does that correspond to the text fragment flag?) suggests at least: 1) Skipping the search for RTL chars in gfxCoreTextShaper::InitTextRun 2) Skipping cluster boundary analysis in gfxPangoFonts 3) Skipping cluster boundary analysis in gfxPlatform::SetupClusterBoundaries I thought there was more on Windows, but not finding it right now.
Assignee | ||
Comment 6•14 years ago
|
||
Hmm. My work in bug 582858 should determine how much of an effect these optimizations have. I have a patch which removes 1b support, and I'll attach it to the bug after some testing.
Assignee | ||
Comment 7•14 years ago
|
||
jst: ping? (This bug blocks bug 240933, which I really hope to get into Beta 4 for testing and feedback).
Comment 8•14 years ago
|
||
Comment on attachment 461134 [details] [diff] [review] Patch (v1) r=jst, sorry for the delay!
Attachment #461134 -
Flags: review?(jst) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #461134 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #461134 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #461134 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 10•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9e887226c511
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Comment 11•14 years ago
|
||
I still think we should have a sane insert API. Followup bug?
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > I still think we should have a sane insert API. Followup bug? -> bug 590557.
Comment 13•13 years ago
|
||
As far as I can tell, this patch actually _hurts_ performance in all sorts of cases. See bug 636655.
Assignee | ||
Comment 14•13 years ago
|
||
This patch was backed out in bug 636655.
Comment 15•13 years ago
|
||
(In reply to comment #14) > This patch was backed out in bug 636655. Reopen?
Comment 16•13 years ago
|
||
What would be the point?
You need to log in
before you can comment on or make changes to this bug.
Description
•