Pasting text is very slow (reflow)

VERIFIED DUPLICATE of bug 28783

Status

()

Core
Layout
P3
normal
VERIFIED DUPLICATE of bug 28783
19 years ago
18 years ago

People

(Reporter: Simon Fraser, Assigned: Joe Francis)

Tracking

({perf})

Trunk
All
Mac System 8.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
Paste a few lines of text into Composer -- it can take several seconds, depending
on how much you are pasting.

The reason it's so slow is that nsHTMLEditRules::WillInsertText() is doing an
InsertTextTxn for each character in the clipboard, which in turn does update
batching etc. But we are going to be reflowing for each insert, which is not
good.

Updated

19 years ago
Target Milestone: M13

Comment 1

19 years ago
I think the deadline for M12 is pretty soon and I know Joe has a lot of stuff he
is trying to land before then so I'm initially triaging this bug to be M13.  It
should be done sooner rather than later since it could involve some
rearchitecting to insert the text in chunks rather than single characters.
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

19 years ago
doh.  thats not hpw it's -supposed- to work.  accepting bug.
(Assignee)

Comment 3

19 years ago
I fixed the "one character pasted at a time" bug.  But I'm leaving this open,
because paste still is butt slow.  Since we can see the caret boogying around, I
assume somehow selection updating isn't getting turned off during the paste,
though from the code it certainly appears to be.  I also have some ideas for
making other improvements to the paste code.
(Assignee)

Comment 4

19 years ago
made some improvements, but leaving open for now (want to work on this some
more).
(Assignee)

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

19 years ago
i've investigated this further.  profiling reveals that reflow is the culprit.
There is a pref that you can add to prefs.js:
user_pref("layout.reflow.asynch",true);
which seems to speed things up quite a bit.  I believe it ends up coellescing a
bunch of reflows that would otherwise be synchronously repeated.

closing this out for now.

Updated

19 years ago
Status: RESOLVED → VERIFIED

Comment 6

19 years ago
verified in 1/13 build.

Updated

19 years ago
Status: VERIFIED → REOPENED

Comment 7

19 years ago
It's still slow.  Reopening to assign to the owners of incremental reflow.

Updated

19 years ago
Assignee: jfrancis → troy
Status: REOPENED → NEW
Component: Editor → Layout
Summary: [Perf] Pasting text is very slow → [Perf] Pasting text is very slow (reflow)

Comment 8

19 years ago
Reassigning to layout since the remaining performance issue is in reflow.  This
may be a DUP of an existing bug on incremental reflow performance.

Updated

19 years ago
Status: NEW → RESOLVED
Last Resolved: 19 years ago19 years ago
Resolution: FIXED → WORKSFORME

Comment 9

19 years ago
I pasted text in and it looked fine (layed out quickly) to me so marking this
WORKSFORME.

Please don't reopen this without some actual numbers to back up the
assertion that reflow is very slow...

Updated

19 years ago
Status: RESOLVED → REOPENED

Comment 10

19 years ago
Reopening -- Joe says he has profiling numbers.  Troy, seeming fast on one test
(how many lines of text did you post?) on one platform is not an indication that
performance is fine everywhere.

Updated

19 years ago
Assignee: troy → jfrancis
Status: REOPENED → NEW

Comment 11

19 years ago
Joe, you said you had profiling numbers showing that reflow was the problem?

Comment 12

19 years ago
Be reasonable. What do you expect me to do with a bug like that? There's no test
case, and no numbers to substantiate the claim...

Updated

19 years ago
Status: NEW → RESOLVED
Last Resolved: 19 years ago19 years ago
Keywords: perf
Summary: [Perf] Pasting text is very slow (reflow) → Pasting text is very slow (reflow)

Comment 13

19 years ago
Editting has had much improvement lately.  Setting to Resolved/WorksForMe
instead of Reopened.

sujay, how is this looking to you?  Please mark Verified if performance is
better.  Or reopen if you think this is impacting Dogfood.

Thanks!

Comment 14

19 years ago
Turns out Jan only checked it with a couple of lines, and not on Linux (not
clear whether she tried it on Windows).

With a debug build from 1/12, it took me roughly 4 seconds to paste 40 lines of
plaintext.  On 1/14 it's just under 3 seconds.  I'd still be interested in
seeing the performance numbers on Mac.  But I'll let Sujay make the call based
on what he sees in the release builds.
(Reporter)

Comment 15

19 years ago
pasting 40 lines takes about 2 seconds on a fast Mac. I think to get a good
handle on this, we should testing large pastes (2 or more pages)

Updated

19 years ago
Status: RESOLVED → REOPENED

Updated

19 years ago
Resolution: WORKSFORME → ---

Comment 16

19 years ago
Good idea.  So here's the test I used, with a 1/14 debug build:
Run mozilla -edit.  View EditorInitPage.html in a plaintext editor, and copy the
whole file (175 lines).  Paste the contents into the editor, timing on a
stopwatch.  I count 18 seconds on my 1/14 debug build before I see anything in
the window.

That seems slow enough to justify reopening.  Lots of people copy/paste whole
web pages.

Comment 17

19 years ago
Clearing resolution.

Comment 18

19 years ago
An interesting question is why is it so slow. Certainly we can lay the exact
same document out very quickly

That suggests to me anyway that we're probably do a whole heck of a lot of
incremental reflows. Most of which are probably unnecessary

How many DOM calls do you make when inserting the text, and how many
ContentInserted/ContentAppended() calls does the content sink make to the frame
construction code?
(Assignee)

Comment 19

19 years ago
*** Bug 21588 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

19 years ago
Target Milestone: M13 → M14
(Assignee)

Comment 20

19 years ago
are we talking with asynch reflow or without?  it's a fact that asynch reflow
helps a lot.  big kudos to nisheeth.  nisheeth, you should use text pasting in
the editor as a performance scenario for you reflow coellescing, if you weren't
already.

paste is still too slow even with nisheeth's work, but relow is no longer the
dominant cause then.  here are some numbers i have:

pasting 5 lines of text (I couldn't use a bunch of text because it overwhelmed my
profiler).  ignore absolute values of the numbers - u cant trust them anyway.
it's the relative values that are important.  This was from a debug mac build,
thursday jan 13, with profiling enabled.

with asynch reflow OFF:
paste took about 550 millisecs.
40% of that time was entirely inside of 48 calls to
PresShell::ProcessReflowCommands()


with asynch on:
paste took about 310 milliseconds
just shy of 30% was entirely inside 60 calls to nsDOMSelection::Collapse()
15% of the time was spent in 433 calls to CreateInstance.  It looks like a lot of
the expense here is hash table lookup in FindFactory().
About 5% was inside 124 calls to nsContentSubtreeIterator::Init()

I noted that a lot of time was spent in CreateInstance, specifically in the case
of the selection code creating a range.  Since nsRangeList is already in bed with
nsRange anyway (it's in the same library, and was including the implementation
header for nsRange already), I tried replacing all those CreateInstances() with
direct calls to NS_NewRange().  This dropped the amount of time we spent in
CreateInstance() by almost 30 milliseconds, and dropped the total number of calls
to it from 433 to 303.

The profiles of the synch/asynch cases are very different.  If asynch reflow is
going to be on by default anytime soon, we should forget about trying to improve
the synch reflow case and concentrate on the asynch case.

I can send/show these profiles to whoever wants them.  You will need a mac and
the MW Profile app (which is included on the codewarrior cds).

To address Troy's question on the number of dom calls: 24 InsertData calls on
text nodes for my 5 line paste, along with .  nsDocument::ContentChanged() got
hit 32 times.  I can improve the editor efficiency (it can make fewer calls).
It's nowhere near as bad as it used to be but it's certainly not minimal.  There
were 10 calls to nsGenericHTMLContainerElement::InsertBefore() and 1 to
RemoveChild().  Given that I have to break the paste out into 5 text nodes and 5
br nodes, this is already minimal.

I'm m14'ing this puppy, and adding mjudge to cc list.

Hope this helps...
(Reporter)

Comment 21

19 years ago
So is async reflow going to become the default at some point? Is it valid to
triage this bug based on performance with async reflow on?

Comment 22

19 years ago
Async reflow was sort of a temporary measure. The reason I asked about how many
reflow commands were getting generated is that Nisheeth is working on reflow
command coalescing which is more general purpose.

Part of the changes have been checked in. He's working on changes to the block
code to handle content appended/inserted and then content changed of text. Once
all those changes are in you should see a noticeable improvement

Nisheeth, let's make sure text frame is changed to use ReflowDirtyChild() just
like the image frame code was changed.
(Assignee)

Comment 23

19 years ago
ok, I'm confused.  How can you possibly coalesce reflows if they aren't
asynchronous?

Comment 24

19 years ago
The pref name is a little misleading. It's asynchronous in either case. The part
that differs is in how the reflow command coalescing works. What we were doing
was have the pres shell coalesce the reflow commands. Now we let the frames
themselves coalesce the reflow commands. This allows greater flexibility, and it
allows coalescing to occur in a hierarchical manner

Comment 25

19 years ago
m16
Target Milestone: M14 → M16
(Assignee)

Updated

19 years ago
Blocks: 26388
(Assignee)

Comment 26

18 years ago
i'm duping this over to 28783 - its the same issue


*** This bug has been marked as a duplicate of 28783 ***
Status: REOPENED → RESOLVED
Last Resolved: 19 years ago18 years ago
Resolution: --- → DUPLICATE

Comment 27

18 years ago
verified in 2/22 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.