Bug 226330

Summary: Double clicking on very large but visually empty DOM can be slow due to selection computation
Product: WebKit Reporter: Liam DeBeasi <ldebeasi>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: emilio, koivisto, marc, phil.moore, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Code reproduction none

Description Liam DeBeasi 2021-05-27 06:55:04 PDT
Created attachment 429874 [details]
Code reproduction

This is a continuation of https://bugs.webkit.org/show_bug.cgi?id=222187 which was partially resolved in STP 125. When using Web Components inside of the Shadow DOM that have CSS Variables set on the host, there is a significant delay in response to clicks/touches. There were fixes in STP 125 that improved this issue, but I am still able to reproduce this issue in other ways.

Steps to reproduce:

1. Open code reproduction in STP 125.
2. Quickly click multiple times on the "Click Me" label (not the checkbox) and you should see Safari freeze up.
3. If you repeat step 2 while taking a Timeline sample, you should see the CPU usage spike significantly. In my tests I clicked about 4-5 times and saw the CPU spike to over 80% on average. 

Expected Behavior:

I would expect Safari to not freeze up when quickly clicking the label multiple times.

Actual Behavior:

Safari freezes up when quickly clicking the label multiple times.

Other Info:

- As noted in https://bugs.webkit.org/show_bug.cgi?id=222187, the example I used is inherently heavy in shadow trees to emphasize the issue. Please see https://github.com/ionic-team/ionic-framework/issues/22951 for more "real world" examples of this impacting applications.
Comment 1 Alexey Proskuryakov 2021-05-27 18:21:22 PDT
Thank you for the report!

Is the performance still worse than pre-iOS 14, or is this about further optimization opportunities? Does this appear to be worse in Safari (STP) than in other browsers?
Comment 2 Liam DeBeasi 2021-05-28 11:37:16 PDT
The performance of single clicking the checkbox in my example is good after https://bugs.webkit.org/show_bug.cgi?id=222187, but multiple clicks is where there are still issues. When doing multiple clicks, the performance in STP 125 is worse than in other browsers and worse than in pre-iOS/Safari 14.
Comment 3 Radar WebKit Bug Importer 2021-06-03 06:56:16 PDT
<rdar://problem/78813010>
Comment 4 marc 2021-06-25 05:30:12 PDT
Hi guys, just wondered if there's been any movement on this?

Thanks,
Comment 5 Philip Moore 2022-06-21 10:34:25 PDT
Be great to understand when this might get resolved, we’ve got a production app https://apps.apple.com/gb/app/mydrivetime-companion-app/id950711705 that’s being heavily impacted and stopping us from releasing a major update that our users need.

Many thanks
Comment 6 marc 2022-06-22 01:10:26 PDT
(In reply to Philip Moore from comment #5)
> Be great to understand when this might get resolved, we’ve got a production
> app https://apps.apple.com/gb/app/mydrivetime-companion-app/id950711705
> that’s being heavily impacted and stopping us from releasing a major update
> that our users need.
> 
> Many thanks

Hi Philip, by the fact this issue hasn't really moved forwards in over a year, I don't think anyone at webkit really cares about it. We had exactly the same issue last year and they've essentially ignored this thread. It seems we are expected to have write overly exuberant lazy loading component code to over come an issue that shouldn't be a problem to start with, as any other browser/device works fine, it's just anything Apple.

This is ridiculous, if I have an issue open for more than a 2 weeks my bosses complain, I think I would have been fired if I had a bug open for over a year.
Comment 7 Liam DeBeasi 2022-06-22 08:22:11 PDT
Hey everyone,

While I know it is frustrating to not have your ticket resolved quickly, comments like the one in https://bugs.webkit.org/show_bug.cgi?id=226330#c6 are not super constructive.

Instead, comments that provide context around the impact of a bug (such as the comment in https://bugs.webkit.org/show_bug.cgi?id=226330#c5) are incredibly helpful. This context can be used to better prioritize issues.

Having worked on a large open source project (though not as large as a browser engine), I can say that it is not always possible to reply to every single issue/comment. Let’s try to keep this thread constructive and focus on identifying the impact this bug has on production apps/websites.
Comment 8 zalan 2022-06-22 21:43:21 PDT
I did a bit of profiling on this and it looks like (after the invalidation fix), this is simply about iterating over that massive amount of content when computing selection boundary. 
The following, simple markup triggers this issue:

<body>
Click Me<input type="checkbox"><br>
    <span></span>
    <span></span>
    <span></span>
    <span></span>
    <span></span>
repeat it for a few thousands of times
</body>

It looks like VisibleSelection needs a more efficient way to figure out the selection end on user action (or in general for that matter).
Comment 9 Antti Koivisto 2022-06-23 09:03:13 PDT
Note that you can work around this issue by preventing the selection with "user-select:none".
Comment 10 zalan 2022-06-23 10:03:31 PDT
(In reply to zalan from comment #8)
> I did a bit of profiling on this and it looks like (after the invalidation
> fix), this is simply about iterating over that massive amount of content
> when computing selection boundary. 
> The following, simple markup triggers this issue:
> 
> <body>
> Click Me<input type="checkbox"><br>
>     <span></span>
>     <span></span>
>     <span></span>
>     <span></span>
>     <span></span>
> repeat it for a few thousands of times
> </body>
> 
> It looks like VisibleSelection needs a more efficient way to figure out the
> selection end on user action (or in general for that matter).
However if there's any "non-visually empty" content somewhere e.g.

<body>
Click Me<input type="checkbox"><br>
     <span></span>
     <span></span>
     <span>!!!some text!!!</span>
     <span></span>
     <span></span>
 repeat it for a few thousand times
</body>

I don't see any spinning anymore. So this is about iterating over massive amount of _visually empty_ content (see nextVisuallyDistinctCandidate()).
Comment 11 Ryosuke Niwa 2022-06-27 14:41:30 PDT
Sounds like this is unrelated to shadow DOM. It's a good bug to address nonetheless.