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.

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 Vector::uncheckedAppend() less in WebCore #18804

Merged

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Oct 7, 2023

ba50d1b

Use Vector::uncheckedAppend() less in WebCore
https://bugs.webkit.org/show_bug.cgi?id=262832

Reviewed by Darin Adler.

Use Vector::uncheckedAppend() less in WebCore and use more efficient alternatives
now that uncheckedAppend() has become an alias for append().

* Source/WebCore/Modules/streams/ReadableStream.cpp:
(WebCore::ReadableStream::tee):
* Source/WebCore/Modules/webxr/WebXRHand.cpp:
(WebCore::WebXRHand::WebXRHand):
* Source/WebCore/PAL/pal/avfoundation/OutputContext.mm:
(PAL::OutputContext::outputDevices const):
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::objectsForIDs const):
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::labelsForNode):
(WebCore::labelForNode):
(WebCore::AccessibilityNodeObject::checkboxOrRadioRect const):
(WebCore::AccessibilityNodeObject::correspondingLabelForControlElement const):
(WebCore::AccessibilityNodeObject::textForLabelElements const):
(WebCore::AccessibilityNodeObject::titleUIElement const):
(): Deleted.
* Source/WebCore/accessibility/AccessibilityNodeObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::children):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::resolveAppends):
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:
(WebCore::ContentExtensions::addUniversalActionsToDFA):
* Source/WebCore/contentextensions/DFAMinimizer.cpp:
(WebCore::ContentExtensions::DFAMinimizer::minimize):
* Source/WebCore/contentextensions/DFANode.cpp:
(WebCore::ContentExtensions::DFANode::actions const):
* Source/WebCore/css/CSSValueList.cpp:
(WebCore::CSSValueContainingVector::copyValues const):
* Source/WebCore/css/CSSVariableData.cpp:
(WebCore::CSSVariableData::CSSVariableData):
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::ComputedStyleExtractor::copyProperties const):
* Source/WebCore/css/StyleProperties.cpp:
(WebCore::StyleProperties::copyProperties const):
* Source/WebCore/css/parser/CSSParserTokenRange.h:
(WebCore::CSSParserTokenRange::size const):
* Source/WebCore/css/typedom/HashMapStylePropertyMapReadOnly.cpp:
(WebCore::HashMapStylePropertyMapReadOnly::entries const):
* Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:
(WebCore::StylePropertyMapReadOnly::reifyValueToVector):
* Source/WebCore/cssjit/SelectorCompiler.cpp:
(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsNthChild):
(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsNthLastChild):
* Source/WebCore/dom/ComposedTreeIterator.h:
(WebCore::ComposedTreeIterator::ComposedTreeIterator):
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::getAttributeNames const):
* Source/WebCore/dom/ElementData.h:
(WebCore::AttributeConstIterator::operator--):
(WebCore::AttributeIteratorAccessor::size const):
* Source/WebCore/dom/ImageOverlay.cpp:
(WebCore::ImageOverlay::updateWithTextRecognitionResult):
* Source/WebCore/dom/SelectorQuery.cpp:
(WebCore::SelectorDataList::SelectorDataList):
* Source/WebCore/dom/TouchList.h:
(WebCore::TouchList::TouchList):
* Source/WebCore/html/FileInputType.cpp:
(WebCore::FileInputType::filesChosen):
* Source/WebCore/html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::textFieldValues const):
* Source/WebCore/html/LinkIconCollector.cpp:
(WebCore::LinkIconCollector::iconsOfTypes):
* Source/WebCore/loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::setActiveContentRuleListActionPatterns):
* Source/WebCore/loader/cache/CachedRawResource.cpp:
(WebCore::CachedRawResource::didAddClient):
* Source/WebCore/page/Page.cpp:
(WebCore::Page::replaceRangesWithText):
* Source/WebCore/page/mac/ServicesOverlayController.mm:
(WebCore::ServicesOverlayController::buildSelectionHighlight):
* Source/WebCore/platform/FileChooser.cpp:
(WebCore::FileChooser::chooseFiles):
* Source/WebCore/platform/SharedBuffer.cpp:
(WebCore::FragmentedSharedBuffer::copy const):
* Source/WebCore/platform/animation/AcceleratedEffectValues.cpp:
(WebCore::AcceleratedEffectValues::AcceleratedEffectValues):
* Source/WebCore/platform/audio/AudioBus.cpp:
(WebCore::AudioBus::AudioBus):
* Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:
(WebCore::AudioDSPKernelProcessor::initialize):
* Source/WebCore/platform/audio/AudioResampler.cpp:
(WebCore::AudioResampler::AudioResampler):
* Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:
(WebCore::DynamicsCompressorKernel::setNumberOfChannels):
* Source/WebCore/platform/audio/MultiChannelResampler.cpp:
(WebCore::MultiChannelResampler::MultiChannelResampler):
* Source/WebCore/platform/graphics/ComplexTextController.cpp:
(WebCore::ComplexTextController::finishConstruction):
* Source/WebCore/platform/graphics/FontCache.cpp:
(WebCore::FontCache::purgeInactiveFontData):
* Source/WebCore/platform/graphics/FontCascade.cpp:
(WebCore::FontCascade::characterSelectionRectsForText const):
* Source/WebCore/platform/graphics/FontCascadeCache.cpp:
(WebCore::makeFontCascadeCacheKey):
* Source/WebCore/platform/graphics/GraphicsLayer.h:
(WebCore::KeyframeValueList::KeyframeValueList):
* Source/WebCore/platform/graphics/PathUtilities.cpp:
(WebCore::PathUtilities::pathsWithShrinkWrappedRects):
* Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:
(WebCore::SourceBufferPrivate::trackBuffersRanges const):
* Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:
(WebCore::extractSinfData):
* Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:
(WebCore::CDMInstanceSessionFairPlayStreamingAVFObjC::copyKeyStatuses const):

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

19c226b

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@cdumez cdumez self-assigned this Oct 7, 2023
@cdumez cdumez added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Oct 7, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 7, 2023
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 7, 2023
@cdumez cdumez force-pushed the 262832_WebCore_less_uncheckedAppend branch from 364f14c to fd2cd45 Compare October 7, 2023 20:35
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 7, 2023
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 7, 2023
@cdumez cdumez force-pushed the 262832_WebCore_less_uncheckedAppend branch from fd2cd45 to df6ba51 Compare October 7, 2023 22:08
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 7, 2023
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 8, 2023
@cdumez cdumez marked this pull request as ready for review October 8, 2023 03:05
@@ -182,8 +182,7 @@ static void addUniversalActionsToDFA(DFA& dfa, UniversalActionSet&& universalAct
ASSERT(!root.actionsLength());
unsigned actionsStart = dfa.actions.size();
dfa.actions.reserveCapacity(dfa.actions.size() + universalActions.size());
for (uint64_t action : universalActions)
dfa.actions.uncheckedAppend(action);
dfa.actions.appendRange(universalActions.begin(), universalActions.end());
Copy link
Member

Choose a reason for hiding this comment

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

Do we benefit from doing reserveCapacity before appendRange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, yes. However, I think we should consider optimizing Vector::appendRange() and reserve capacity at least when the input iterators are random access iterators (and std::distance() can return the size in constant time). I can look into this but likely in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following up in #18839.

for (auto& value : *this)
builder.uncheckedAppend(const_cast<CSSValue&>(value));
return builder;
return WTF::map<4>(*this, [](auto& value) -> Ref<CSSValue> {
Copy link
Member

@darinadler darinadler Oct 8, 2023

Choose a reason for hiding this comment

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

The idea is that CSSValueListBuilder could change its inline capacity in the future. The hardcoded 4 here doesn’t seem great, especially if it will make working but inefficient code in case of mismatch. I suppose it’s OK if compilation will just fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fails to build at the moment:

Source/WebCore/css/CSSValueList.cpp:214:12: error: no viable conversion from returned value of type 'Vector<[...], 0UL aka 0, [2 * ...]>' to function return type 'Vector<[...], 4, [2 * ...]>'

That said, it is probably better to use a global constant to be safe. Nothing stops someones from updating the Vector class to convert silently.

auto& block = result.blocks[index];

size_t index = 0;
Vector<FontSizeAdjustmentState> elementsToAdjust = WTF::compactMap(result.blocks, [&](auto& block) -> std::optional<FontSizeAdjustmentState> {
Copy link
Member

Choose a reason for hiding this comment

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

auto?

Vector<UserContentURLPattern> patternVector;
patternVector.reserveInitialCapacity(pair.value.size());
for (auto& patternString : pair.value) {
Vector<UserContentURLPattern> patternVector = WTF::compactMap(pair.value, [](auto& patternString) -> std::optional<UserContentURLPattern> {
Copy link
Member

Choose a reason for hiding this comment

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

auto?

cgRects.reserveInitialCapacity(m_currentSelectionRects.size());

for (auto& rect : m_currentSelectionRects) {
Vector<CGRect> cgRects = WTF::map(m_currentSelectionRects, [&](auto& rect) -> CGRect {
Copy link
Member

Choose a reason for hiding this comment

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

auto?

@cdumez cdumez force-pushed the 262832_WebCore_less_uncheckedAppend branch from df6ba51 to 19c226b Compare October 8, 2023 23:50
@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Oct 8, 2023
https://bugs.webkit.org/show_bug.cgi?id=262832

Reviewed by Darin Adler.

Use Vector::uncheckedAppend() less in WebCore and use more efficient alternatives
now that uncheckedAppend() has become an alias for append().

* Source/WebCore/Modules/streams/ReadableStream.cpp:
(WebCore::ReadableStream::tee):
* Source/WebCore/Modules/webxr/WebXRHand.cpp:
(WebCore::WebXRHand::WebXRHand):
* Source/WebCore/PAL/pal/avfoundation/OutputContext.mm:
(PAL::OutputContext::outputDevices const):
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::objectsForIDs const):
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::labelsForNode):
(WebCore::labelForNode):
(WebCore::AccessibilityNodeObject::checkboxOrRadioRect const):
(WebCore::AccessibilityNodeObject::correspondingLabelForControlElement const):
(WebCore::AccessibilityNodeObject::textForLabelElements const):
(WebCore::AccessibilityNodeObject::titleUIElement const):
(): Deleted.
* Source/WebCore/accessibility/AccessibilityNodeObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::children):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::resolveAppends):
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:
(WebCore::ContentExtensions::addUniversalActionsToDFA):
* Source/WebCore/contentextensions/DFAMinimizer.cpp:
(WebCore::ContentExtensions::DFAMinimizer::minimize):
* Source/WebCore/contentextensions/DFANode.cpp:
(WebCore::ContentExtensions::DFANode::actions const):
* Source/WebCore/css/CSSValueList.cpp:
(WebCore::CSSValueContainingVector::copyValues const):
* Source/WebCore/css/CSSVariableData.cpp:
(WebCore::CSSVariableData::CSSVariableData):
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::ComputedStyleExtractor::copyProperties const):
* Source/WebCore/css/StyleProperties.cpp:
(WebCore::StyleProperties::copyProperties const):
* Source/WebCore/css/parser/CSSParserTokenRange.h:
(WebCore::CSSParserTokenRange::size const):
* Source/WebCore/css/typedom/HashMapStylePropertyMapReadOnly.cpp:
(WebCore::HashMapStylePropertyMapReadOnly::entries const):
* Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:
(WebCore::StylePropertyMapReadOnly::reifyValueToVector):
* Source/WebCore/cssjit/SelectorCompiler.cpp:
(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsNthChild):
(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsNthLastChild):
* Source/WebCore/dom/ComposedTreeIterator.h:
(WebCore::ComposedTreeIterator::ComposedTreeIterator):
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::getAttributeNames const):
* Source/WebCore/dom/ElementData.h:
(WebCore::AttributeConstIterator::operator--):
(WebCore::AttributeIteratorAccessor::size const):
* Source/WebCore/dom/ImageOverlay.cpp:
(WebCore::ImageOverlay::updateWithTextRecognitionResult):
* Source/WebCore/dom/SelectorQuery.cpp:
(WebCore::SelectorDataList::SelectorDataList):
* Source/WebCore/dom/TouchList.h:
(WebCore::TouchList::TouchList):
* Source/WebCore/html/FileInputType.cpp:
(WebCore::FileInputType::filesChosen):
* Source/WebCore/html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::textFieldValues const):
* Source/WebCore/html/LinkIconCollector.cpp:
(WebCore::LinkIconCollector::iconsOfTypes):
* Source/WebCore/loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::setActiveContentRuleListActionPatterns):
* Source/WebCore/loader/cache/CachedRawResource.cpp:
(WebCore::CachedRawResource::didAddClient):
* Source/WebCore/page/Page.cpp:
(WebCore::Page::replaceRangesWithText):
* Source/WebCore/page/mac/ServicesOverlayController.mm:
(WebCore::ServicesOverlayController::buildSelectionHighlight):
* Source/WebCore/platform/FileChooser.cpp:
(WebCore::FileChooser::chooseFiles):
* Source/WebCore/platform/SharedBuffer.cpp:
(WebCore::FragmentedSharedBuffer::copy const):
* Source/WebCore/platform/animation/AcceleratedEffectValues.cpp:
(WebCore::AcceleratedEffectValues::AcceleratedEffectValues):
* Source/WebCore/platform/audio/AudioBus.cpp:
(WebCore::AudioBus::AudioBus):
* Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:
(WebCore::AudioDSPKernelProcessor::initialize):
* Source/WebCore/platform/audio/AudioResampler.cpp:
(WebCore::AudioResampler::AudioResampler):
* Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:
(WebCore::DynamicsCompressorKernel::setNumberOfChannels):
* Source/WebCore/platform/audio/MultiChannelResampler.cpp:
(WebCore::MultiChannelResampler::MultiChannelResampler):
* Source/WebCore/platform/graphics/ComplexTextController.cpp:
(WebCore::ComplexTextController::finishConstruction):
* Source/WebCore/platform/graphics/FontCache.cpp:
(WebCore::FontCache::purgeInactiveFontData):
* Source/WebCore/platform/graphics/FontCascade.cpp:
(WebCore::FontCascade::characterSelectionRectsForText const):
* Source/WebCore/platform/graphics/FontCascadeCache.cpp:
(WebCore::makeFontCascadeCacheKey):
* Source/WebCore/platform/graphics/GraphicsLayer.h:
(WebCore::KeyframeValueList::KeyframeValueList):
* Source/WebCore/platform/graphics/PathUtilities.cpp:
(WebCore::PathUtilities::pathsWithShrinkWrappedRects):
* Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:
(WebCore::SourceBufferPrivate::trackBuffersRanges const):
* Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:
(WebCore::extractSinfData):
* Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:
(WebCore::CDMInstanceSessionFairPlayStreamingAVFObjC::copyKeyStatuses const):

Canonical link: https://commits.webkit.org/269062@main
@webkit-commit-queue webkit-commit-queue force-pushed the 262832_WebCore_less_uncheckedAppend branch from 19c226b to ba50d1b Compare October 9, 2023 00:56
@webkit-commit-queue
Copy link
Collaborator

Committed 269062@main (ba50d1b): https://commits.webkit.org/269062@main

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

@webkit-commit-queue webkit-commit-queue merged commit ba50d1b into WebKit:main Oct 9, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
5 participants