Bug 234842

Created attachment 448298 [details] Patch

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32240

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

It seems both clever to do this, but also makes me wish we had some other definition of "too expensive" other than using ApproximateTime. Will this be OK from an automated testing point of view? Seems like we could have different test results on different speed computers, and other such things, whereas some other definition of complexity (counting something, perhaps) would give us more reproducible results, that could have the same effect.

Another way to think about it is that if we stick with the ApproximateTime strategy, as computers get faster and faster, WebKit becomes more and more willing to waste up to 64ms doing even more accurate style invalidation that it did on the previous generation, but it still doesn’t have enough benefit to be worth it. Maybe not a practical problem?

I like the use of time measurement since it actually gives a highly relevant definition for "expensive". The style system is complex enough that "count number of times something happens" approaches are difficult to make capture all scenarios. This is really meant to protect against pathological cases, the current 64ms limit is difficult to hit unless you are specifically trying. This probably won't affect any tests normal tests. I think we need a counter in addition to the timer though. The current patch would make debugging annoying since setting a breakpoint inside the timed section would affects subsequent execution even in trivial cases. Also I'm still considering other options.

Comment on attachment 448298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448298&action=review > LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/has-complexity.html:39 > +const start = performance.now(); nit: You're not using this. I assume you want to remove this? Logging (end - start) would cause flakiness in the test results.

<rdar://problem/87397176>

Created attachment 449053 [details] Patch

Comment on attachment 449053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449053&action=review > Source/WebCore/style/ChildChangeInvalidation.cpp:72 > + // FIXME: We should cache this state accross invalidations instead of just testing a single sibling. Typo: across > LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/has-complexity.html:29 > +const yellow = 'rgb(128, 0, 128)'; This is actually purple. > LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/has-complexity.html:48 > +for (let i = 0; i < count - 1; ++i) { Why -1 here? > LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/has-complexity.html:65 > +testColor(`After appending div with ${count} elements. This should not time out.`, yellow); You should probably rename yellow to purple since that's what you use in the CSS.

> This is actually purple. ohno > > LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/has-complexity.html:48 > > +for (let i = 0; i < count - 1; ++i) { > > Why -1 here? we add the last element separately so that gives exactly count elements.

Created attachment 449149 [details] Patch for landing

Created attachment 449150 [details] Patch

Committed r288012 (246038@main): <https://commits.webkit.org/246038@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449150 [details].

Comment on attachment 449150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449150&action=review > Source/WebCore/style/ChildChangeInvalidation.h:31 > +#include <wtf/HashSet.h> Pretty sure this can be done with Forward.h instead, and might be worth it. Can define a type and declare a function involving it without the HashSet.h header. The hash table headers are super commonly used but also huge, so I’m never sure how important it is to keep them out of other headers.

*** Bug 235840 has been marked as a duplicate of this bug. ***