Closed Bug 582852 Opened 9 years ago Closed 9 years ago

Optimize large text node editing

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b5

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

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.
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #461134 - Flags: review?(jst)
I thought we also had layout optimizations for "ascii" textframes.  Is that no longer the case?
General note: we can get some of the benefits of this by just having a sane insert api on textfragments.
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.
> 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.
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.
See Also: → 582858
jst: ping?  (This bug blocks bug 240933, which I really hope to get into Beta 4 for testing and feedback).
Comment on attachment 461134 [details] [diff] [review]
Patch (v1)

r=jst, sorry for the delay!
Attachment #461134 - Flags: review?(jst) → review+
Attachment #461134 - Flags: approval2.0?
Attachment #461134 - Flags: approval2.0? → approval2.0+
Attached patch For check-inSplinter Review
Attachment #461134 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9e887226c511
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
I still think we should have a sane insert API.  Followup bug?
(In reply to comment #11)
> I still think we should have a sane insert API.  Followup bug?

-> bug 590557.
As far as I can tell, this patch actually _hurts_ performance in all sorts of cases.  See bug 636655.
Depends on: 636655
This patch was backed out in bug 636655.
(In reply to comment #14)
> This patch was backed out in bug 636655.

Reopen?
You need to log in before you can comment on or make changes to this bug.