Bug 239876

Created attachment 458552 [details] Patch

Created attachment 458553 [details] Microbenchmark

Created attachment 458555 [details] Patch Add RefCountedVector.h to CMakeLists.txt

Comment on attachment 458555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458555&action=review Antti should probably look at this but I made comments since there are a few non-WebKit idioms in here. > Source/WTF/wtf/RefCountedVector.h:41 > + static Ref<RefCountedVector> create(size_t size) Maybe the parameter could be optional with = 0;, given how it is used. > Source/WebCore/dom/ClassCollection.cpp:41 > +RefPtr<ClassCollection> ClassCollection::create(ContainerNode& rootNode, const AtomString& classNames) This cannot return null so this change seems like a regression, we should return a Ref<>, not a RefPtr<>. > Source/WebCore/dom/ClassCollection.h:73 > +inline void ClassCollection::invalidateCacheIfNeeded(const ClassChangeVector* classChanges) This function assumes classChanges pointer is non null so we should pass a reference instead of a pointer. > Source/WebCore/dom/CollectionIndexCache.h:47 > + Ref<CachedListType> ensureValidCache(const Collection& collection) Given that this isn't a one liner, we should move it out of the class (put it below in this file) and mark it as inline. > Source/WebCore/dom/NodeRareData.h:128 > + ALWAYS_INLINE RefPtr<ClassCollection> addCachedClassCollection(ContainerNode& node, const AtomString& classNames) Why does this return a RefPtr<> and not a Ref<>. It does not look like it can return null? > Source/WebCore/dom/NodeRareData.h:191 > + void removeCachedClassCollection(RefPtr<ClassCollection> collection, const AtomString& classNames) It seems this is unnecessarily doing ref counting churn for the collection, this should likely take a `ClassCollection&` in argument. The call site should just pass `*this`. Note that RefPtr<> was not only inefficient but also confusing since this function cannot be called with null. > Source/WebCore/dom/SelectorQuery.cpp:171 > + const CSSSelector& selector = *m_selectors.first().selector; auto& > Source/WebCore/dom/SelectorQuery.cpp:174 > + const AtomString& className = selector.value(); auto& > Source/WebCore/dom/StaticNodeList.h:84 > + unsigned length() const override; could be marked as final > Source/WebCore/dom/StaticNodeList.h:85 > + Element* item(unsigned index) const override; ditto. > Source/WebCore/style/ClassChangeInvalidation.cpp:84 > +void ClassChangeInvalidation::computeInvalidation(const ClassChangeVector* classChanges) Looks like classChanges is assumed to be non null so we should pass a reference, not a raw pointer: const ClassChangeVector& > Source/WebCore/style/ClassChangeInvalidation.cpp:90 > + for (const auto& classChange : *classChanges) { We don't normally use `const auto`, the previous code was correct and more WebKit-like. It was already const and the previous code was more concise. > Source/WebCore/style/ClassChangeInvalidation.cpp:114 > + for (const auto& classChange : *classChanges) { Ditto. > Source/WebCore/style/ClassChangeInvalidation.h:60 > + ASSERT(classChanges); No, please pass classChanges by reference and drop this assertion.

Comment on attachment 458555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458555&action=review > Source/WebCore/dom/Element.cpp:2007 > + std::unique_ptr<ClassChangeVector> classChanges; This needs a pointless heap allocation. It should be std::optional. > Source/WebCore/dom/Element.cpp:2009 > + classChanges = makeUnique<ClassChangeVector>(Style::ClassChangeInvalidation::computeClassChanges(oldClassNames, newClassNames)); It would be better to factor computeClassChanges out from style code (like ClassChangeVector already was) if it is going to be used outside it. > Source/WebCore/dom/Element.cpp:-2005 > + if (needsStyleInvalidation) { > + Style::ClassChangeInvalidation styleInvalidation(*this, classChanges.get()); > + elementData()->setClassNames(WTFMove(newClassNames)); > + } else > elementData()->setClassNames(WTFMove(newClassNames)); > - } You could avoid two paths for setClassName by having ClassChangeInvalidation do nothing when uninitialized classChanges is passed in (keeping the existing ClassChangeInvalidation::m_isEnabled bit).

> > + std::unique_ptr<ClassChangeVector> classChanges; > > This needs a pointless heap allocation. It should be std::optional. Or just plain ClassChangeVector. An empty vector signifies nothing needs to be done.

Created attachment 458609 [details] Patch Adjust test and apply all suggestions except passing ClassChangeVector by reference and keeping m_isEnabled.

Thank you both for the very thorough review. Really appreciate all the comments! (In reply to Chris Dumez from comment #4) > > Source/WebCore/dom/NodeRareData.h:128 > > + ALWAYS_INLINE RefPtr<ClassCollection> addCachedClassCollection(ContainerNode& node, const AtomString& classNames) > > Why does this return a RefPtr<> and not a Ref<>. It does not look like it > can return null? It indeed doesn't make sense and was used just for the `fastAdd(classNames, nullptr)` thing, which doesn't justify it at all and was replaced with ensure(). > > Source/WebCore/style/ClassChangeInvalidation.h:60 > > + ASSERT(classChanges); > > No, please pass classChanges by reference and drop this assertion. Given the way ClassChangeVector is used (in a loop), I don't see how we can std::move it, so passing it by reference means copying the content, right? I'm a bit hesitant to do that given it's on a very hot path, hence the pointer. Is there a way to drop ASSERT(classChanges) while avoiding copying the vector? (In reply to Antti Koivisto from comment #5) > > Source/WebCore/dom/Element.cpp:-2005 > > + if (needsStyleInvalidation) { > > + Style::ClassChangeInvalidation styleInvalidation(*this, classChanges.get()); > > + elementData()->setClassNames(WTFMove(newClassNames)); > > + } else > > elementData()->setClassNames(WTFMove(newClassNames)); > > - } > > You could avoid two paths for setClassName by having ClassChangeInvalidation > do nothing when uninitialized classChanges is passed in (keeping the > existing ClassChangeInvalidation::m_isEnabled bit). Yeah that's possible, but I try to avoid computeClassChanges() if it won't be used (which I presume most of the times), and for that I require needsStyleInvalidation(), which is non-trivial and I'm attempting to avoid adding an calling it twice on the hot path.

Comment on attachment 458609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458609&action=review I do not understand what copies you are talking about. A reference is like a pointer that cannot be null, it doesn't involve copying. > Source/WebCore/style/ClassChangeInvalidation.cpp:38 > +void ClassChangeInvalidation::computeInvalidation(const ClassChangeVector* classChanges) Please pass a `const ClassChangeVector&` > Source/WebCore/style/ClassChangeInvalidation.cpp:44 > + for (auto& classChange : *classChanges) { classChanges > Source/WebCore/style/ClassChangeInvalidation.cpp:68 > + for (auto& classChange : *classChanges) { classChanges > Source/WebCore/style/ClassChangeInvalidation.h:44 > + void computeInvalidation(const ClassChangeVector*); const ClassChangeVector& > Source/WebCore/style/ClassChangeInvalidation.h:54 > +inline ClassChangeInvalidation::ClassChangeInvalidation(Element& element, const ClassChangeVector* classChanges) const ClassChangeVector& > Source/WebCore/style/ClassChangeInvalidation.h:58 > + ASSERT(classChanges); No longer needed.

Created attachment 458619 [details] Patch Pass ClassChangeVector by reference and fix --debug build.

Comment on attachment 458619 [details] Patch (In reply to Chris Dumez from comment #9) > Comment on attachment 458609 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458609&action=review > > I do not understand what copies you are talking about. A reference is like a pointer that cannot be null, it doesn't involve copying. Oh wow, I've got basics all wrong, that's embarrassing. Thanks Chris! > LayoutTests/ChangeLog:8 > + * intersection-observer/root-element-deleted.html: This makes me think index cache change to keep Ref<Element> instead of Element* should probably be reverted, it's unsafe and not actually necessary.

(In reply to Alexey Shvayka from comment #11) > Comment on attachment 458619 [details] > Patch > > > LayoutTests/ChangeLog:8 > > + * intersection-observer/root-element-deleted.html: > > This makes me think index cache change to keep Ref<Element> instead of > Element* should probably be reverted, it's unsafe and not actually necessary. ?? Ref will keep Element alive whereas Element* won't so can end up doing use-after-free. It's definitely safer to use Ref.

> Yeah that's possible, but I try to avoid computeClassChanges() if it won't > be used (which I presume most of the times), and for that I require > needsStyleInvalidation(), which is non-trivial and I'm attempting to avoid > adding an calling it twice on the hot path. I understand that. I meant ChangeChangeVector classChanges; if (needsComputeClassChange) classChanges = computeClassChanges(...); ClassChangeInvalidation invalidation(classChanges); setClassName(...); where ClassChangeInvalidation simply doesn't do anything with empty vector.

Comment on attachment 458619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458619&action=review > Source/WTF/ChangeLog:10 > + * wtf/RefCountedVector.h: Added. Can you provide a description of what this new data structure is and when / why one would use it?

Comment on attachment 458619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458619&action=review > Source/WTF/wtf/RefCountedVector.h:35 > +class RefCountedVector final : public RefCounted<RefCountedVector<T>>, public Vector<T> { What's the point of this class?? > Source/WebCore/dom/CollectionIndexCache.h:40 > + using CachedListType = RefCountedVector<Ref<NodeType>>; We can't use Ref here. This will result in a leak since we don't automatically clear collection cache when a subtree is detached. r- because of this.

Comment on attachment 458619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458619&action=review > Source/WebCore/dom/ClassCollection.h:83 > + if (hasOneRef()) > + removeFromCache(); Deleting self like this is a dangerous pattern. Let's make this function return a boolean indicating that this object needs to be removed instead. > Source/WebCore/dom/Node.cpp:1003 > + for (auto* node = this; node; node = node->parentNode()) { Please use RefPtr instead. > Source/WebCore/dom/Node.cpp:1977 > +void NodeListsNodeData::invalidateCachesForClassAttribute(const ClassChangeVector& classChanges) We should probably rename the other one to invalidateCachesForNonClassAttribute > Source/WebCore/dom/Node.cpp:1983 > + Vector<ClassCollection*> cachedClassCollections; > + cachedClassCollections.reserveInitialCapacity(m_cachedClassCollections.size()); > + for (auto& classCollection : m_cachedClassCollections.values()) > + cachedClassCollections.uncheckedAppend(classCollection.ptr()); > + Instead of always allocating a vector of collection, why not just call invalidateCacheIfNeeded and store the collection to a vector only if the function returns boolean indicating that the deletion is necessary. > Source/WebCore/dom/Node.h:81 > + AtomStringImpl* className { }; Can we just store AtomString instead? Storing a raw pointer like this seems rather dangerous. This seems to be an over-optimization to me.

<rdar://problem/92827069>

(In reply to Ryosuke Niwa from comment #15) Hey Ryosuke, sorry for the delay (I took some time off), and thank you for thorough review. Always appreciated. > > Source/WebCore/dom/CollectionIndexCache.h:40 > > + using CachedListType = RefCountedVector<Ref<NodeType>>; > > We can't use Ref here. This will result in a leak since we don't > automatically clear collection cache when a subtree is detached. > r- because of this. Could you please give me a hint on how to repro this leak? I've tried a bunch of cases with detaching ancestor / parent before / after acquiring a valid cache for ClassCollection, while trying to invalidate it via changing attributes / children. The cache always seems to be invalidated (yet again, I've tried only ClassCollection): when a node is detached, Node::invalidateNodeListAndCollectionCachesInAncestors() is called on its parent and goes all the way up. > > LayoutTests/ChangeLog:8 > > + * intersection-observer/root-element-deleted.html: > > This makes me think index cache change to keep Ref<Element> instead of > Element* should probably be reverted, it's unsafe and not actually necessary. Expanding on this comment: I've used Ref<NodeType> so the vector could be shared (to avoid copying, since it's quite large in Speedo2) between a static querySelectorAll() NodeList and the cache of live getElementsByClassName() collection, even though the latter could get around with raw C++ pointer or WeakPtr.