Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Setting the value of a textarea is much slower in WebKit than it is in Chromium #6411

Merged
merged 1 commit into from Nov 12, 2022
Merged

Setting the value of a textarea is much slower in WebKit than it is in Chromium #6411

merged 1 commit into from Nov 12, 2022

Conversation

darinadler
Copy link
Member

@darinadler darinadler commented Nov 11, 2022 •
edited by webkit-early-warning-system

610bbdb

Setting the value of a textarea is much slower in WebKit than it is in Chromium
https://bugs.webkit.org/show_bug.cgi?id=247739
rdar://problem/102218029

Reviewed by Alexey Proskuryakov.

These changes make the micro-benchmark of setting the text of a textarea about 4x faster.

* Source/WTF/wtf/text/StringView.cpp:
(WTF::makeStringBySimplifyingNewLinesSlowCase): Added. Implements a faster algorithm for
standardizing line endings as opposed to making two passes through the string.

* Source/WTF/wtf/text/StringView.h:
(WTF::makeStringBySimplifyingNewLines): Added a high-speed check for '\r' characters before doing
the slower algorithm to standardize line separators. Most strings won't have any at all, and none
of the ones in the benchmark do.

Canonical link: https://commits.webkit.org/256596@main

2d6c86c

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk   🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
  🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
  🛠 🧪 jsc ✅ 🛠 tv   🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-mips-tests

@darinadler darinadler self-assigned this Nov 11, 2022
@darinadler darinadler added Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) Safari Technology Preview labels Nov 11, 2022
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 2d6c86c)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk   🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
  🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
  🛠 🧪 jsc ✅ 🛠 tv   🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-mips-tests

aproskuryakov
aproskuryakov approved these changes Nov 11, 2022
View reviewed changes
Source/WTF/wtf/text/StringView.cpp
@@ -447,6 +447,35 @@ int codePointCompare(StringView lhs, StringView rhs)
return codePointCompare(lhs.characters16(), lhs.length(), rhs.characters16(), rhs.length());
}

template<typename CharacterType> static String makeStringBySimplifyingNewLinesSlowCase(const String& string, unsigned firstCarriageReturn)
{
unsigned length = string.length();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth ASSERTing some sanity for firstCarriageReturn. Chances are this function is so much of a special case that no one will ever try to misuse it, so not feeling strongly about the need for sanity checks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel comfortable leaving this as is.

Source/WTF/wtf/text/StringView.cpp
auto result = String::createUninitialized(length, resultCharacters);
memcpy(resultCharacters, characters, firstCarriageReturn * sizeof(CharacterType));
for (unsigned i = firstCarriageReturn; i < length; ++i) {
if (characters[i] != '\r')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight concern about whether the existing function implemented the correct algorithm. Do we know for certain what normalization is supposed to be done? I've seen some weird newline combinations in the past (\n\r, \r\r\n are treated as a single newline by some software).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am certain the function is doing the correct normalization. I researched this originally and still remember learning this was exactly what is needed.

@darinadler darinadler added the merge-queue Applied to send a pull request to merge-queue label Nov 12, 2022
@darinadler
Setting the value of a textarea is much slower in WebKit than it is i…
610bbdb 
…n Chromium

https://bugs.webkit.org/show_bug.cgi?id=247739
rdar://problem/102218029

Reviewed by Alexey Proskuryakov.

These changes make the micro-benchmark of setting the text of a textarea about 4x faster.

* Source/WTF/wtf/text/StringView.cpp:
(WTF::makeStringBySimplifyingNewLinesSlowCase): Added. Implements a faster algorithm for
standardizing line endings as opposed to making two passes through the string.

* Source/WTF/wtf/text/StringView.h:
(WTF::makeStringBySimplifyingNewLines): Added a high-speed check for '\r' characters before doing
the slower algorithm to standardize line separators. Most strings won't have any at all, and none
of the ones in the benchmark do.

Canonical link: https://commits.webkit.org/256596@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/Setting-the-value-of-a-textarea-is-much-slower-in-WebKit-than-it-is-in-Chromium branch from 2d6c86c to 610bbdb Compare November 12, 2022 01:09
@webkit-commit-queue
Copy link
Collaborator

Committed 256596@main (610bbdb): https://commits.webkit.org/256596@main

Reviewed commits have been landed. Closing PR #6411 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 610bbdb into WebKit:main Nov 12, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 12, 2022
@darinadler darinadler deleted the eng/Setting-the-value-of-a-textarea-is-much-slower-in-WebKit-than-it-is-in-Chromium branch November 13, 2022 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aproskuryakov aproskuryakov aproskuryakov approved these changes

@cdumez cdumez Awaiting requested review from cdumez

@MenloDorian MenloDorian Awaiting requested review from MenloDorian

@Constellation Constellation Awaiting requested review from Constellation

Assignees

@darinadler darinadler

Labels
Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.)
Projects
None yet
Milestone
No milestone
4 participants
@darinadler @webkit-early-warning-system @webkit-commit-queue @aproskuryakov