Summary: | [GPU Process] Unique RenderingResourceIdentifiers Part 9: Finish migrating RemoteResourceCache to QualifiedRenderingResourceIdentifier | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bfulgham, sabouhallawa, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 217638 | ||||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2021-10-07 23:34:12 PDT
Created attachment 440576 [details]
Patch
Created attachment 440640 [details]
Patch
Created attachment 440884 [details]
Patch
Created attachment 440909 [details]
Patch
Comment on attachment 440909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440909&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:47 > +class ResourceHeapImpl : public ResourceHeap { This name does not reveal the difference between ResourceHeapImpl and QualifiedResourceHeap. Can't it be named LocalResourceHeap or RenderingResourceHeap or something else? > Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:51 > + m_resources.add(renderingResourceIdentifier, RefPtr<ImageBuffer>(WTFMove(imageBuffer))); Why do we need to"add()" a RefPtr<> even though the input is a Ref<>? Also I think RefPtr<ImageBuffer>(WTFMove(imageBuffer)) can be just WTFMove(imageBuffer). > Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:96 > using Resource = Variant<RefPtr<ImageBuffer>, RefPtr<NativeImage>, RefPtr<Font>>; I think this should be a Variant of Ref<> resources since we never add a null pointer to it. > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:49 > + add<WebCore::ImageBuffer>(renderingResourceIdentifier, WTFMove(imageBuffer), m_imageBufferCount); Do we need to use explicit template specialization here? Can't it be implicit add(renderingResourceIdentifier, WTFMove(imageBuffer), m_imageBufferCount); > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:109 > + return remove<WebCore::ImageBuffer>(renderingResourceIdentifier, m_imageBufferCount); Do we need to make an early return here: if (!m_imageBufferCount) return false; > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:124 > + m_resources.removeIf([] (const auto& resource) { Do we need to make an early return here: if (!m_fontCount) return; > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:136 > + ++counter; Maybe we need to assert here ASSERT(m_resources.size() == m_imageBufferCount + m_nativeImageCount + m_fontCount); > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:146 > + if (!WTF::holds_alternative<RefPtr<T>>(iterator->value)) > + return nullptr; I think we should ASSERT(WTF::holds_alternative...) since it should be a bug in our code to get an Image with the ResourceIdentifier of a font for example. > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:153 > + auto iterator = m_resources.find(renderingResourceIdentifier); Maybe we need to assert here ASSERT(counter); > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:157 > + if (!WTF::holds_alternative<RefPtr<T>>(iterator->value)) > + return false; I think we should ASSERT(WTF::holds_alternative...) since it should be a bug in our code to remove an Image with the ResourceIdentifier of a font for example. > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:164 > + using Resource = Variant<RefPtr<WebCore::ImageBuffer>, RefPtr<WebCore::NativeImage>, RefPtr<WebCore::Font>>; I think this should be a Variant of Ref<> resources since we never add a null pointer to it. > Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:169 > + HashMap<QualifiedRenderingResourceIdentifier, Resource> m_resources; > + WebCore::ProcessIdentifier m_webProcessIdentifier; > + unsigned m_imageBufferCount { 0 }; > + unsigned m_nativeImageCount { 0 }; > + unsigned m_fontCount { 0 }; What is the benefit of having a HashMap of the variant and three counters over having three HashMaps one for each resource type? Comment on attachment 440909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440909&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:96 >> using Resource = Variant<RefPtr<ImageBuffer>, RefPtr<NativeImage>, RefPtr<Font>>; > > I think this should be a Variant of Ref<> resources since we never add a null pointer to it. Yep! Alex just taught me how to do this. Very cool. >> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:109 >> + return remove<WebCore::ImageBuffer>(renderingResourceIdentifier, m_imageBufferCount); > > Do we need to make an early return here: > > if (!m_imageBufferCount) > return false; Good idea. I should probably put it in remove<>() though, so it's shared. >> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:136 >> + ++counter; > > Maybe we need to assert here > > ASSERT(m_resources.size() == m_imageBufferCount + m_nativeImageCount + m_fontCount); Yeah, this is a good idea since it's an invariant. I'll put this in a helper and call the helper in a bunch of different places. >> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:157 >> + return false; > > I think we should ASSERT(WTF::holds_alternative...) since it should be a bug in our code to remove an Image with the ResourceIdentifier of a font for example. I thought so too, but in this specific case it's actually not: bool RemoteResourceCache::maybeRemoveResource(...) { ... if (m_resourceHeap.removeImageBuffer(renderingResourceIdentifier) || m_resourceHeap.removeNativeImage(renderingResourceIdentifier)) { updateHasActiveDrawables(); return true; } if (m_resourceHeap.removeFont(renderingResourceIdentifier)) return true; ... } add() and get() should ASSERT() though. >> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:169 >> + unsigned m_fontCount { 0 }; > > What is the benefit of having a HashMap of the variant and three counters over having three HashMaps one for each resource type? https://bugs.webkit.org/show_bug.cgi?id=231411#c7 Three reasons: 1. Less wasted space in the maps 2. It's a bug if differently typed resources shared the same RRI 3. Simon says "it’s semantically better" Created attachment 440980 [details]
Patch for committing
Created attachment 440985 [details]
Patch for committing
Created attachment 440992 [details]
Patch for committing
Committed r284048 (242877@main): <https://commits.webkit.org/242877@main> |