search

WebKit / Speedometer

An open source repository for the Speedometer benchmark
Other
592 stars 70 forks source link

Codemirror: platform-custom code in editor #347

Closed camillobruni closed 6 months ago

camillobruni commented 8 months ago

There are quite a few places where we have very android and iOS specific code that is not executed elsewhere. I don't think that's ideal, I'd still propose to integrate my navigator sanitizer (see #336) to get rid of this entirely.

rniwa commented 8 months ago

Since the sanitizer script doesn't seem to be happening in near future, can we put up a PR to remove this style manually?

julienw commented 8 months ago

I'm not worried about platform-specific checks but more about browser-specific checks. Examples: https://github.com/search?q=org%3Acodemirror%20browser.chrome&type=code https://github.com/search?q=org%3Acodemirror%20browser.ie&type=code https://github.com/search?q=org%3Acodemirror%20browser.gecko&type=code https://github.com/search?q=org%3Acodemirror%20browser.webkit&type=code

We need to check whether these occurrences are in the path of the tests.

bgrins commented 8 months ago

It's worth looking into the linked examples above to see which of them is getting executed and has a meaningful performance impact, but based on personal experience with contentEditable it might be impossible to remove browser-specific checks without breaking the editor itself (or setting up a future breakage if/when we expand the test to hit a currently-unused code path).

I agree that in general we don't want browser-specific code in the benchmark, but in this case it may be inevitable given the nature of editors (part of the reason these libraries get popular is because they smooth over cross browser behavior differences).

camillobruni commented 7 months ago

I did measure 100 local runs with setting defaults on the browser object and there are some minor changes to the score. Now I can't fully tell at this point whether the functionality is equivalent or if we just run different code.

Safari TP (Release 186 (Safari 17.4, WebKit 19618.1.10.1.1)):

Screenshot 2024-01-24 at 18 48 53

Safari (Version 17.3 (19617.2.4.11.8)):

Screenshot 2024-01-24 at 18 49 33

Firefox Nightly (124.0a1 (2024-01-23) (64-Bit)):

Screenshot 2024-01-24 at 18 51 36

Firefox (121.0.1 (64-bit)):

Screenshot 2024-01-24 at 18 50 58

Chrome Canary (123.0.6262.0):

Screenshot 2024-01-24 at 18 50 17

Chrome Stable (120.0.6099.234):

Screenshot 2024-01-24 at 18 52 00
bgrins commented 7 months ago

There was some back and forth discussion today highlighting that while in general we don't want this kind of workaround, the nature of contentEditable can make them necessary and removing them may actually make cross-browser behavior more inconsistent (and less representative of code in the wild). So the plan is to do some more investigation (especially with the Safari conditions, cc @rniwa), to inform a decision about whether we want to change any code or simply document why this is an exception.

rniwa commented 7 months ago

Is there a PR for this that I can test out?

camillobruni commented 7 months ago

I've duplicated the editor code in my branch: https://github.com/camillobruni/Speedometer/tree/2024-01-24_custom_editor

And it contains the patched browser object: https://github.com/camillobruni/Speedometer/blob/2024-01-24_custom_editor/resources/editors_sanitized/dist/assets/codemirror-ba10726a.js

rniwa commented 6 months ago

I've gotten 5 samples of run for each browser's. The results look largely unchanged although maybe ~0.5% or so better in "after" measurements across the board. Given this, I'm fine with going ahead with this change. Is there an open PR for it?

Chrome 122.0.6261.57 == Before == 28.9 ± 0.81 29.0 ± 0.62 29.9 ± 0.83 30.0 ± 0.51 29.3 ± 0.65

== After ==

29.7 ± 0.81 30.5 ± 0.98 30.3 ± 1.0 29.9 ± 0.90 30.1 ± 0.91

Firefox 123.0 == Before == 28.0 ± 0.81 27.9 ± 0.60 28.0 ± 0.70 28.0 ± 0.73 27.8 ± 0.62

== After == 28.4 ± 0.84 28.8 ± 0.71 28.9 ± 0.79 28.5 ± 0.76 29.1 ± 0.68

Safari Technology Preview 188 == Before == 29.6 ± 1.1 30.2 ± 1.1 29.6 ± 1.3 30.1 ± 1.1 30.4 ± 1.2

== After == 31.3 ± 1.4 30.7 ± 1.1 30.8 ± 1.2 30.3 ± 0.96 30.8 ± 1.2

speedometer-editor-change.zip

bgrins commented 6 months ago

Given this, I'm fine with going ahead with this change. Is there an open PR for it?

As I mentioned in https://github.com/WebKit/Speedometer/issues/347#issuecomment-1900868393, I prefer the behavior as-is, since contentEditable creates the need for browser-specific quirks (part of the reason editor libraries are popular are so these get abstracted away). So we'd really have to validate not just any performance difference but also investigate whether removing those changes the actual functionality. Given that there doesn't seem to be a strong performance difference one way or the other, I'd like to just close this issue (perhaps with a note in the README indicating that we avoid browser-specific behavior in general, but in this particular test we are allowing it).

rniwa commented 6 months ago

Given this, I'm fine with going ahead with this change. Is there an open PR for it?

As I mentioned in #347 (comment), I prefer the behavior as-is, since contentEditable creates the need for browser-specific quirks (part of the reason editor libraries are popular are so these get abstracted away). So we'd really have to validate not just any performance difference but also investigate whether removing those changes the actual functionality. Given that there doesn't seem to be a strong performance difference one way or the other, I'd like to just close this issue (perhaps with a note in the README indicating that we avoid browser-specific behavior in general, but in this particular test we are allowing it).

I'm fine with that outcome as well. Sounds like maybe we're leaning more towards closing this then?

rniwa commented 6 months ago

Per today's meeting, closing this issue in favor of keeping the current code.