Testcase takes 22s to copy-paste from browser to a text editor. With a tiny modification, it takes 200ms.
Categories
(Core :: Web Painting, enhancement)
Tracking
()
People
(Reporter: mayankleoboy1, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
- Type the following in a new tab : " data:text/html, <html style contenteditable spellcheck="false"> " . This will create a text editor type thingy.
- Open the slow testcase in the browser
- Select all the text and copy it (Ctrl+A, Ctrl + C)
- Paste the text into the text-editor from #1
- Now repeat the steps with the fast testcase
Fast: https://share.firefox.dev/4fDxgIg (174ms)
Slow: https://share.firefox.dev/4daAYaT (22s)
The fast testcase is the original testcase. To make it slow, I deleted all the <div> and </div> tags.
Maybe something to improve for the slow testcase?
|
Reporter | |
Comment 1•1 year ago
|
||
|
|
Reporter | |
Comment 2•1 year ago
|
||
|
|
Reporter | |
Comment 3•1 year ago
|
||
:jjaschke, I am ni? you as you may have some opinion on this.
|
||
Comment 4•1 year ago
|
||
Interesting case, thank you for posting. The 'slow' case is a node with a lot of child nodes, hence ComputeIndexOf() becomes slow and shows up in the profile. I wrote some technical details in Bug 1503581 comment 14.
Since I have looking into bug 1503581, I tried this test case with that WIP patch applied. It turns out that said patch adds roughly one order of magnitude to the slow case (the fast testcase stays the same. It doesn't use the cache because of the 1k child node threshold). The reason is, that while pasting, the document's generation changes permanently, forcing the cache to be rebuilt constantly.
I think it may be possible to add some batching there:
While inserting, we're only adding nodes into a single parent node. So, theoretically, we don't have to clear the cache (this is more a question than a statement!). Note that I have limited knowledge of the Editor internals.
If we would add batching to the AutoPlaceholderBatch which would clear the cache once and then prohibit clearing the cache, we would keep the cache alive during the paste operation.
I gave that a try, and the slow test case was about as fast as the fast one (<1s).
However, IIUC inserting nodes during paste could trigger DOM mutations, which would currently be handled during paste. These would have to be batched as well.
Masayuki, what do you think about this (also bug 1503581)? I'm sure I've missed many edge cases and I don't know if AutoPlaceholderBatch is the right place to add the batching. Do you think it makes sense to look deeper into this?
Well, I think it's fine to put it into HTMLEditor::HTMLWithContextInserter::InsertContents because the insertion occur sequentially. It might break some cases if legacy mutation event listeners intercepts something. However, in such tricky case, web apps should charge with the result by themselves. (FYI: Chrome/Safari does not dispatch events while their builtin editor touches the DOM.)
|
|
Reporter | |
Comment 6•10 months ago
|
||
This is much improved now.
Slow testcase: https://share.firefox.dev/3UoJFXd
Fast testcase: https://share.firefox.dev/4m17BM0
|
||
Comment 7•10 months ago
|
||
Well, it spends in HTMLEditor::RemoveEmptyNodesIn() in 52%. Ideally, we should make HTMLEditor immediately delete empty nodes when it appears instead of handling all of them at the post processing. However, it requires a lot of changes in HTMLEditor and I don't have a good idea to do that safely. So, I think that we cannot fix this immediately. On the other hand, we could improve the logic to consider whether a node is empty.
|
|
||
Comment 8•10 months ago
|
||
Well, the method scanning forward. So, when it appends a new parent, we could delete its previous sibling from the cache because non-empty check requires only the last non-empty node in the same container.
|
|
||
Comment 9•10 months ago
|
||
Odd, I cannot get the similar result of the slow testcase profiling. I don't see HTMLEditor::RemoveEmptyNodesIn() in my environment...
|
|
||
Comment 10•10 months ago
|
||
I wrote patches to make RemoveEmptyNodesIn() avoid to the worst cases. However, it does not appear in the profile in my environment. Could you test the test build whether it improves the response? The job is here. Unfortunately, the patches do not improve the speedometer scores.
|
|
Reporter | |
Comment 11•10 months ago
|
||
I dont repro it too. :( . Sorry for the misdirection.
I will close this as fixed. Thanks for all your work on this!
(I got a different STR which could be a different bug. Will file a new bug for that.)
Description
•