Bug 231281 – [GPU Process] Unique RenderingResourceIdentifiers Part 1: Uniquify at entry points

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[GPU Process] Unique RenderingResourceIdentifiers Part 1: Uniquify at entry points

[Myles C. Maxfield]

Created attachment 440334 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83920780>

[Simon Fraser (smfr)]

Comment on attachment 440334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440334&action=review

> Source/WebKit/ChangeLog:26
> +        This patch is the first step: It introduces the QualifiedRenderingResourceIdentifier
> +        type, which holds the tuple described above, and makes all the GPU process message
> +        receivers immediately turn any RenderingResourceIdentifiers they received from the
> +        web processes into QualifiedRenderingResourceIdentifiers. Not only do the identifiers
> +        immediately get packed, but each message receiver now has an implementation function
> +        which only accepts the QualifiedRenderingResourceIdentifier, so we can't make a
> +        mistake and use the unpacked one accidentally.

Is this problem unique to RenderingResourceIdentifier or should we make a solution that can be used for other types of identifiers?

It's really a "process coalition-wide identifier" or something, right?

[Myles C. Maxfield]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 440334 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440334&action=review
> 
> > Source/WebKit/ChangeLog:26
> > +        This patch is the first step: It introduces the QualifiedRenderingResourceIdentifier
> > +        type, which holds the tuple described above, and makes all the GPU process message
> > +        receivers immediately turn any RenderingResourceIdentifiers they received from the
> > +        web processes into QualifiedRenderingResourceIdentifiers. Not only do the identifiers
> > +        immediately get packed, but each message receiver now has an implementation function
> > +        which only accepts the QualifiedRenderingResourceIdentifier, so we can't make a
> > +        mistake and use the unpacked one accidentally.
> 
> Is this problem unique to RenderingResourceIdentifier or should we make a
> solution that can be used for other types of identifiers?
> 
> It's really a "process coalition-wide identifier" or something, right?

Oh, I assumed it was just for RenderingResourceIdentifier because the radar rdar://71012915 specifically mentions that type.

Do you have any suggestions for how to make this more generic? Make the struct templated?

[Cameron McCormack (:heycam)]

Comment on attachment 440334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440334&action=review

> Source/WebKit/GPUProcess/graphics/QualifiedRenderingResourceIdentifier.h:54
> +    QualifiedRenderingResourceIdentifier(WTF::HashTableDeletedValueType hashTableDeletedValueType)
> +        : renderingResourceIdentifier(hashTableDeletedValueType)

You could drop the argument name and use `HashTableDeletedValue` in the initializer.

Since we can use webProcessIdentifier uninitialized in operator== and operator<< (and maybe add?), we should initialize it with the deleted value type here too.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:216
> +void RemoteGraphicsContextGL::paintCompositedResultsToQualifiedCanvas(QualifiedRenderingResourceIdentifier imageBuffer, CompletionHandler<void()>&& completionHandler)

"QualifiedCanvas" sounds a bit weird. Do you plan to eventually remove the original messages and rename the "Qualified" ones to remove that word?

[Myles C. Maxfield]

Comment on attachment 440334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440334&action=review

>> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:216
>> +void RemoteGraphicsContextGL::paintCompositedResultsToQualifiedCanvas(QualifiedRenderingResourceIdentifier imageBuffer, CompletionHandler<void()>&& completionHandler)
> 
> "QualifiedCanvas" sounds a bit weird. Do you plan to eventually remove the original messages and rename the "Qualified" ones to remove that word?

I don't. The design that I think makes the most sense is that the web processes speak WebCoreRenderingResourceIdentifier, and so that's what they would send to the GPU process, but the GPU Process would augment those identifiers with a ProcessIdentifier upon receipt.

Maybe "QualifiedCanvasIdentifier" is better?

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #5)
> You could drop the argument name and use `HashTableDeletedValue` in the
> initializer.
> 
> Since we can use webProcessIdentifier uninitialized in operator== and
> operator<< (and maybe add?), we should initialize it with the deleted value
> type here too.

Not necessary, as Myles points out ObjectIdentifier has a default constructor that's fine here.

[Cameron McCormack (:heycam)]

(In reply to Myles C. Maxfield from comment #6)
> I don't. The design that I think makes the most sense is that the web
> processes speak WebCoreRenderingResourceIdentifier, and so that's what they
> would send to the GPU process, but the GPU Process would augment those
> identifiers with a ProcessIdentifier upon receipt.
> 
> Maybe "QualifiedCanvasIdentifier" is better?

Yes, that reads better to me.  You suggested offline that the full name "paintCompositedResultsWithQualifiedCanvasIdentifier", which seems fine.  Or "paintCompositedResultsToCanvasWithQualifiedIdentifier".  If it's not getting too wordy.

[Myles C. Maxfield]

Created attachment 440453 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 440453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440453&action=review

> Source/WebKit/ChangeLog:48
> +        (WebKit::RemoteGraphicsContextGL::paintRenderingResultsToQualifiedCanvas):

ChangeLog needs updating.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:82
> +inline TextStream& operator<<(TextStream& ts, QualifiedResourceIdentifier<T> resourceIdentifier)

`const QualifiedResourceIdentifier<T>&`? Now that the type is templated we don't know how big the concrete type is, so this should perhaps take a const reference. (And does that mean the constructor should take a `T&&`?)

> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:102
> +    static unsigned hash(WebKit::QualifiedResourceIdentifier<T> identifier)

Ditto.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:107
> +    static bool equal(WebKit::QualifiedResourceIdentifier<T> a, WebKit::QualifiedResourceIdentifier<T> b)

Ditto.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:112
> +    static const bool safeToCompareToEmptyOrDeleted = true;

Should this be defined to be equal to `DefaultHash<T>::safeToCompareToEmptyOrDeleted`?

> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:115
> +template<typename T> struct HashTraits<WebKit::QualifiedResourceIdentifier<T>> : SimpleClassHashTraits<WebKit::QualifiedResourceIdentifier<T>> {

And should `HashTraits<QualifiedResourceIdentifier<T>>::emptyValueIsZero` be `HashTraits<T>::emptyValueIsZero`?

These questions might not need to be addressed if we define the QualifiedResourceIdentifier::resourceIdentifier to be of type `ObjectIdentifier<T>` instead of T. Is that generic enough for your purposes?

[Myles C. Maxfield]

Comment on attachment 440453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440453&action=review

>> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:115
>> +template<typename T> struct HashTraits<WebKit::QualifiedResourceIdentifier<T>> : SimpleClassHashTraits<WebKit::QualifiedResourceIdentifier<T>> {
> 
> And should `HashTraits<QualifiedResourceIdentifier<T>>::emptyValueIsZero` be `HashTraits<T>::emptyValueIsZero`?
> 
> These questions might not need to be addressed if we define the QualifiedResourceIdentifier::resourceIdentifier to be of type `ObjectIdentifier<T>` instead of T. Is that generic enough for your purposes?

It is, but I think there's real value in being able to say `QualifiedResourceIdentifier<RenderingResourceIdentifier>` instead of `QualifiedResourceIdentifier<RenderingResourceIdentifierType>` and just relying implicitly on the fact that RenderingResourceIdentifier is a ObjectIdentifier<RenderingResourceIdentifierType>. So I'll just change the signatures of these functions to use references.

[Myles C. Maxfield]

Created attachment 440470 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 440470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440470&action=review

Non-reviewer r=me.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:161
> +    void createQualifiedImageBuffer(const WebCore::FloatSize& logicalSize, WebCore::RenderingMode, float resolutionScale, const WebCore::DestinationColorSpace&, WebCore::PixelFormat, QualifiedRenderingResourceIdentifier);
> +    void getDataURLForQualifiedImageBuffer(const String& mimeType, std::optional<double> quality, WebCore::PreserveResolution, QualifiedRenderingResourceIdentifier, CompletionHandler<void(String&&)>&&);
> +    void getDataForQualifiedImageBuffer(const String& mimeType, std::optional<double> quality, QualifiedRenderingResourceIdentifier, CompletionHandler<void(Vector<uint8_t>&&)>&&);
> +    void getShareableBitmapForQualifiedImageBuffer(QualifiedRenderingResourceIdentifier, WebCore::PreserveResolution, CompletionHandler<void(ShareableBitmap::Handle&&)>&&);
> +    void cacheQualifiedNativeImage(const ShareableBitmap::Handle&, QualifiedRenderingResourceIdentifier);
> +    void didCreateQualifiedSharedDisplayListHandle(WebCore::DisplayList::ItemBufferIdentifier, const SharedMemory::IPCHandle&, QualifiedRenderingResourceIdentifier destinationBufferIdentifier);
> +    void releaseQualifiedRemoteResource(QualifiedRenderingResourceIdentifier, uint64_t useCount);
> +    void cacheQualifiedFont(Ref<WebCore::Font>&&, QualifiedRenderingResourceIdentifier);

Can these all be renamed to fit the "paintRenderingResultsWithQualifiedCanvasIdentifier" naming scheme too? I guess I wasn't clear that it wasn't just that one method that might be better renamed. (They key thing being that "qualified canvas identifier" sounds like it's the identifier which is qualified, which is the case, but in the earlier name "paintRenderingResultsToQualifiedCanvas" it sounds like a "qualified canvas" is a thing, which it isn't.)

These are getting a bit wordy, though, so I'll leave it up to you.

[Chris Dumez]

Comment on attachment 440470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440470&action=review

> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:40
> +struct QualifiedResourceIdentifier {

I don't understand. It seems you worked hard to write a generic templated class but then:
1. You use "ResourceIdentifier" in the name
2. You put in the WebKit/GPUProcess folder
3. You include RenderingResourceIdentifier.h and add an alias for it.

I think this should either:
1. Be a class very specific to RenderingResourceIdentifier and then it can stay here (and we don't need templating).
or preferably
2. We make this class / header truly generic and reusable. Which would entail:
a. Move it to WebCore/platform, where ProcessIdentifier lives
b. Using a more generic name (e.g. QualifiedIdentifier or QualifiedObjectIdentifier if you choose to use an ObjectIdentifier<T> instead of a T).
c. Not include RenderingResourceIdentifier.h or have aliases for it in this header. This can be done elsewhere in the GPUProcess code.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceIdentifier.h:82
> +inline TextStream& operator<<(TextStream& ts, const QualifiedResourceIdentifier<T>& resourceIdentifier)

What is TextStream uses for? If it is for logging, do we really want 2 identifiers concatenated without any kind of separator?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:507
> +void RemoteRenderingBackend::cacheQualifiedNativeImage(const ShareableBitmap::Handle& handle, QualifiedRenderingResourceIdentifier nativeImageResourceIdentifier)

Why do we need a separate function? It seems this is only called by cacheNativeImage() (which is a one-liner).

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:531
> +void RemoteRenderingBackend::cacheQualifiedFont(Ref<Font>&& font, QualifiedRenderingResourceIdentifier fontResourceIdentifier)

Same comment as above.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:551
> +void RemoteRenderingBackend::releaseQualifiedRemoteResource(QualifiedRenderingResourceIdentifier renderingResourceIdentifier, uint64_t useCount)

Same comment as above.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:576
> +void RemoteRenderingBackend::didCreateQualifiedSharedDisplayListHandle(DisplayList::ItemBufferIdentifier itemBufferIdentifier, const SharedMemory::IPCHandle& handle, QualifiedRenderingResourceIdentifier)

Same comment as above.

[Myles C. Maxfield]

Comment on attachment 440470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440470&action=review

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:507
>> +void RemoteRenderingBackend::cacheQualifiedNativeImage(const ShareableBitmap::Handle& handle, QualifiedRenderingResourceIdentifier nativeImageResourceIdentifier)
> 
> Why do we need a separate function? It seems this is only called by cacheNativeImage() (which is a one-liner).

(Chris and I discussed this offline.)

[Chris Dumez]

(In reply to Myles C. Maxfield from comment #15)
> Comment on attachment 440470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440470&action=review
> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:507
> >> +void RemoteRenderingBackend::cacheQualifiedNativeImage(const ShareableBitmap::Handle& handle, QualifiedRenderingResourceIdentifier nativeImageResourceIdentifier)
> > 
> > Why do we need a separate function? It seems this is only called by cacheNativeImage() (which is a one-liner).
> 
> (Chris and I discussed this offline.)

Yep, my bad. I withdraw those comments.

[Myles C. Maxfield]

Created attachment 440547 [details]
Patch

[Chris Dumez]

Comment on attachment 440547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440547&action=review

r=me. It may have been nice to use ProcessQualifiedIdentifier and use an ObjectIdentifier<T>. I am not sure how useful this could be for other types.

> Source/WebCore/platform/ProcessQualified.h:107
> +}

Would be nice to add this here as we usually do.
 // namespace WebCore

> Source/WebCore/platform/ProcessQualified.h:130
> +}

Would be nice to add this here as we usually do.
 // namespace WTF

[Myles C. Maxfield]

Comment on attachment 440547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440547&action=review

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h:105
> +    void paintRenderingResultsToCanvasWithQualifiedIdentifier(QualifiedRenderingResourceIdentifier, CompletionHandler<void()>&&);
> +    void paintCompositedResultsToCanvasWithQualifiedIdentifier(QualifiedRenderingResourceIdentifier, CompletionHandler<void()>&&);

These should be private.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h:111
> +    void paintPixelBufferToImageBuffer(std::optional<WebCore::PixelBuffer>&&, QualifiedRenderingResourceIdentifier, CompletionHandler<void()>&&);

Ditto.

[Myles C. Maxfield]

Committed r283755 (242676@main): <https://commits.webkit.org/242676@main>