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.
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
Use find8 and find16 in AdaptiveStringSearcher #28403
Use find8 and find16 in AdaptiveStringSearcher #28403
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
// Special-case looking for the 0 char in other than one-byte strings. | ||
// memchr mostly fails in this case due to every other byte being 0 in text | ||
// that is mostly ascii characters. | ||
for (int i = index; i < maxN; ++i) { | ||
if (!subjectPtr[i]) | ||
return i; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this no longer apply? Or is it just not worth the upfront runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer applies since we use find16 instead of memchr.
if constexpr (sizeof(PatternChar) == 2 && sizeof(SubjectChar) == 1) { | ||
if (!isLatin1(patternFirstChar)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why could we handle this case before and not now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the previous one was using memchr and a hack for both 16 / 8 bit cases. Now the logic is completely different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
https://bugs.webkit.org/show_bug.cgi?id=274010 rdar://127894478 Reviewed by Mark Lam. We should just use find8 and find16 in AdaptiveStringSearcher. Since we have find16, we do not need to use memchr hack here. * Source/WTF/wtf/text/AdaptiveStringSearcher.h: (WTF::AdaptiveStringSearcherBase::findFirstCharacter): (WTF::AdaptiveStringSearcherBase::alignDown): Deleted. (WTF::AdaptiveStringSearcherBase::getHighestValueByte): Deleted. Canonical link: https://commits.webkit.org/278644@main
b8d9e6a
to
31005e5
Compare
Committed 278644@main (31005e5): https://commits.webkit.org/278644@main Reviewed commits have been landed. Closing PR #28403 and removing active labels. |
31005e5
b8d9e6a
π jsc-armv7π§ͺ jsc-armv7-tests