Ticket #6438 (closed Bug: fixed)

Opened 4 years ago

Last modified 3 years ago

IE: Performance issue when typing inside an element with many children

Reported by: tobiasz.cudnik Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.5.1
Component: General Version: 3.0
Keywords: IE Cc: zhanyi1022@…

Description

There is a performance issue when typing inside an element with many children. Reproduced in IE8.

Steps

  1. Paste content from the attachment into the editor
  2. Place cursor after last character, but not outside of the paragraph.
  3. Start typing

Expected: Text is typed as usual.

Actual: There is a serious hangup and CPU consumption.

Attachments

content.txt (7.3 KB) - added by tobiasz.cudnik 4 years ago.
6438.patch (1.7 KB) - added by garry.yao 4 years ago.
6438_2.patch (2.5 KB) - added by garry.yao 3 years ago.
6438_3.patch (1.7 KB) - added by garry.yao 3 years ago.
6438_4.patch (5.4 KB) - added by garry.yao 3 years ago.

Change History

Changed 4 years ago by tobiasz.cudnik

comment:1 Changed 4 years ago by tobiasz.cudnik

  • Status changed from new to confirmed

comment:2 Changed 4 years ago by garry.yao

I consider this issue quite important as it's interfered editing experience, it's a regression caused by [5452], the slowness shown in the below stack rooted by 'compareEndPoints' which apparently we cannot avoid.

onkeydown -> saveSelection -> getSelection -> getBoundaryInformation -> IERange::compareEndPoints

Changed 4 years ago by garry.yao

comment:3 Changed 4 years ago by garry.yao

  • Status changed from confirmed to review
  • Owner set to garry.yao

Well we also don't want to revert [5452], so the patch doesn't really resolve the issue but at least optimize such case, making the editor usable.

If the proposed approach is rejected, this ticket should be no longer be kept in 3.4.2.

comment:4 Changed 4 years ago by Saare

  • Status changed from review to review_failed

It's now definitely better, but we should investigate it some more, we may find a way to fix the bug rather than working around it.

comment:5 Changed 4 years ago by garry.yao

  • Milestone changed from CKEditor 3.4.2 to CKEditor 3.5

comment:6 Changed 3 years ago by fredck

  • Milestone changed from CKEditor 3.5 to CKeditor 3.4.3

Changed 3 years ago by garry.yao

comment:7 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review

Ok, I've managed to simplify things quite a lot, yes, I'm not joking, Fred would be the best reviewer here.

comment:8 Changed 3 years ago by fredck

Well... I'm always strongly afraid when we see part of our code being cut that way. For some good reason it's there.

I'm not reviewing it right now. I'm just leaving a warning notice for now.

comment:9 Changed 3 years ago by fredck

  • Status changed from review to review_failed

The assumptions from my previous comment unfortunately became true. It was as easy as use SVN blame to check why we have those lines of code there. The patch causes a regression of #5778. Just the reproduction steps need attention:

  1. Load an editor with enough content to make the scrollbar available in the editor.
  2. Because of another editor issue, do not use the scrollbar to scroll-down, but do that with the mouse wheel.
  3. Right-click at the last paragraph.

After patch results: the contents will scroll to the top.

comment:10 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review

Indeed, I'd checked #5778 but as it's saying "affecting all IE" that I missed IE8 (the only affected in effect), but the major purpose is to revert part of [3329], so other parts are innocent.

Changed 3 years ago by garry.yao

comment:11 follow-up: ↓ 12 Changed 3 years ago by fredck

  • Status changed from review to review_failed
  • Version changed from 3.4.1 to 3.0
  • Milestone changed from CKEditor 3.4.3 to CKEditor 3.5.1

In the current code, it's clear that we wanted to disable selection saving while either the mouse button or the keyboard key is down, re-enabling it when they're up. There must surely be a reason for that, but unfortunately I can't recall it (my fault for missing comments).

I think we should not proceed with the patch if we're not able to have the TC that originated that code, otherwise we'll be certainly introducing a regression. I'm removing the review just because of this, so we can give more thinking to this issue.

Couldn't we instead try to have better performance on the selection saving code?

comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 3 years ago by garry.yao

Replying to fredck:

In the current code, it's clear that we wanted to disable selection saving while either the mouse button or the keyboard key is down, re-enabling it when they're up. There must surely be a reason for that, but unfortunately I can't recall it (my fault for missing comments).

With the patch, selection saving happens now on editor blur (and it should), so it makes no sense of keeping the original logic.

Couldn't we instead try to have better performance on the selection saving code?

Haven't find any possibility yet.

comment:13 in reply to: ↑ 12 Changed 3 years ago by fredck

Replying to garry.yao:

With the patch, selection saving happens now on editor blur (and it should), so it makes no sense of keeping the original logic.

That was the original idea when I've implemented the selection saving logic as well, but, if I remember it well, there are some situations where the related blur events are not fired, making the original logic necessary.

As I've said, this is all to be confirmed.

comment:14 follow-up: ↓ 15 Changed 3 years ago by garry.yao

  • Status changed from review_failed to assigned

comment:15 in reply to: ↑ 14 Changed 3 years ago by jayhamiltoniv

Replying to garry.yao:

Hello,

My company is close to being production ready with our first release built on top of CKEditor. Is this correction still targeted for 3.5.1 or do we need to adjust our timeline?

Thanks, Jay

Changed 3 years ago by garry.yao

comment:16 Changed 3 years ago by garry.yao

  • Status changed from assigned to review

Proposing binary search for getBoundaryInformation reduce the complexity to O(logn).

comment:17 Changed 3 years ago by garry.yao

Note that 6438_4.patch's NOT conflicted with 6438_3.patch, it would be better if both patches could be accepted.

comment:18 Changed 3 years ago by Saare

It's an R+ for me, but I think we need a third reviewer here. Also, it might be for the best to wait with that ticket to the next milestone, to have more time to test it and use the editor with it before releasing to the public.

comment:19 Changed 3 years ago by fredck

  • Status changed from review to review_passed

There are a few things that could be simplified in the code, but it's generally acceptable. It's a great solution for the problem.

It would be nice to have a few comments to describe what's going on in the code, so it'll be easier to understand the logic. Other than that, there are a few points that need coding style attention.

Better to have a lot of attention on IE during the tests because of this one.

comment:20 Changed 3 years ago by garry.yao

  • Status changed from review_passed to closed
  • Resolution set to fixed

Thanks for zhanyi for the original idea,fixed with [6337].

comment:21 Changed 3 years ago by garry.yao

  • Cc zhanyi1022@… added

comment:22 follow-up: ↓ 23 Changed 3 years ago by alfonsoml

Is this patch the same one that was mentioned in #6837 ?

comment:23 in reply to: ↑ 22 Changed 3 years ago by garry.yao

Replying to alfonsoml:

Is this patch the same one that was mentioned in #6837 ?

Yes, originate from this forum post.

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy