Bug 217752 – [WebXR] Move OpenXR calls off the main thread

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

[Sergio Villar Senin]

[WebXR] Move OpenXR calls off the main thread

[Sergio Villar Senin]

Created attachment 411442 [details]
Patch

[Sergio Villar Senin]

Created attachment 411443 [details]
Patch

[Sergio Villar Senin]

Gentle ping for reviewers

[Zan Dobersek]

Comment on attachment 411443 [details]
Patch

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

> Source/WebCore/platform/xr/PlatformXR.h:79
> -    void enumerateImmersiveXRDevices(CompletionHandler<void(const Vector<std::unique_ptr<Device>>&)>&&);
> +    using DeviceList = Vector<std::unique_ptr<Device>>;
> +    void enumerateImmersiveXRDevices(CompletionHandler<void(DeviceList*)>&&);

Is there a functionality-based reason behind this change? It's more assuring having to deal with an empty vector than a null pointer, plus you probably then need extra handling of a non-null pointer to an empty vector.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:198
> +#else
> +{
> +}
> +#endif // USE_OPENXR

I would recommend separately guarding the m_workQueue initialization and the internals of the constructor implementation.

Guarding the body of the constructor this way leans too much into the preprocessor IMO, in the sense that the preprocessor is used to considerably change the formation of the implementation file, instead of just changing behavior of specific elements of the implementation (in this case the constructor's initialization list and body).

[Sergio Villar Senin]

Comment on attachment 411443 [details]
Patch

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

>> Source/WebCore/platform/xr/PlatformXR.h:79
>> +    void enumerateImmersiveXRDevices(CompletionHandler<void(DeviceList*)>&&);
> 
> Is there a functionality-based reason behind this change? It's more assuring having to deal with an empty vector than a null pointer, plus you probably then need extra handling of a non-null pointer to an empty vector.

Just to avoid creating an empty vector in the error case. It does not require any extra handling but we can switch back to empty vectors.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:198
>> +#endif // USE_OPENXR
> 
> I would recommend separately guarding the m_workQueue initialization and the internals of the constructor implementation.
> 
> Guarding the body of the constructor this way leans too much into the preprocessor IMO, in the sense that the preprocessor is used to considerably change the formation of the implementation file, instead of just changing behavior of specific elements of the implementation (in this case the constructor's initialization list and body).

Fair enough, I'll change it.

[youenn fablet]

Comment on attachment 411443 [details]
Patch

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

>>> Source/WebCore/platform/xr/PlatformXR.h:79
>>> +    void enumerateImmersiveXRDevices(CompletionHandler<void(DeviceList*)>&&);
>> 
>> Is there a functionality-based reason behind this change? It's more assuring having to deal with an empty vector than a null pointer, plus you probably then need extra handling of a non-null pointer to an empty vector.
> 
> Just to avoid creating an empty vector in the error case. It does not require any extra handling but we can switch back to empty vectors.

An empty vector is cheap so it is reasonable to use a const ref than a const pointer.

[Sergio Villar Senin]

Created attachment 411747 [details]
Patch

[Sam Weinig]

Comment on attachment 411747 [details]
Patch

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

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:66
> +    WorkQueue& m_queue;

Can this be strong reference (Ref<WorkQueue>) to the WorkQueue, or are you afraid of a cycle?

[Sergio Villar Senin]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 411747 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411747&action=review
> 
> > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:66
> > +    WorkQueue& m_queue;
> 
> Can this be strong reference (Ref<WorkQueue>) to the WorkQueue, or are you
> afraid of a cycle?

There shouldn't be any cycle because devices do not hold references to the PlatformXR::Instance. However I'm using a reference because the ownership of the queue is not transferred. According to [1] (I know it's a bit old)

"
* If ownership and lifetime are guaranteed, a data member can be a raw reference or pointer.
* If the class needs to hold ownership or guarantee lifetime, the data member should be a Ref or RefPtr."

That's why I preferred to use a reference, but I don't have a strong opinion here, so I can switch to a Ref if you think it's more appropriate. 

[1] https://webkit.org/blog/5381/refptr-basics/

[youenn fablet]

Comment on attachment 411747 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:94
> +            });

Why not callOnMainThread(WTFMove(callback))?

> Source/WebCore/platform/xr/PlatformXR.h:78
> +    using DeviceList = Vector<std::unique_ptr<Device>>;

Can we use a Vector<UniqueRef<>>?

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:232
> +                callback(emptyList);

We can probably use callback({ })

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254
> +            m_immersiveXRDevices.append(makeUnique<OpenXRDevice>(systemId, m_impl->xrInstance(), m_impl->queue()));

How are we sure this is valid here? Should we instead use a weak pointer?
Something like:
m_impl->queue().dispatch([weakThis = makeWeakPtr(this)...]
...
callOnMainThread([weakThis = WTFMove(weakThis)...] {
    if (!weakThis) {
       callback({ });
       return;
    }
   ...
});

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:257
> +                    callback(m_immersiveXRDevices);

Ditto here.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:262
> +        callbackOnExit.release();

Just my taste but it reads better if callbackOnExit.release(); is called just before moving callback in callOnMainThread lambda.

[Sergio Villar Senin]

Comment on attachment 411747 [details]
Patch

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

Thanks for the review! Uploading a new patch soon.

>> Source/WebCore/Modules/webxr/WebXRSystem.cpp:94
>> +            });
> 
> Why not callOnMainThread(WTFMove(callback))?

Oh indeed. I had some debug code inside that block, that's why I didn't realize about it.

>> Source/WebCore/platform/xr/PlatformXR.h:78
>> +    using DeviceList = Vector<std::unique_ptr<Device>>;
> 
> Can we use a Vector<UniqueRef<>>?

Makes total sense to me.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:232
>> +                callback(emptyList);
> 
> We can probably use callback({ })

Right, now that we're returning a const reference it should be indeed possible.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254
>> +            m_immersiveXRDevices.append(makeUnique<OpenXRDevice>(systemId, m_impl->xrInstance(), m_impl->queue()));
> 
> How are we sure this is valid here? Should we instead use a weak pointer?
> Something like:
> m_impl->queue().dispatch([weakThis = makeWeakPtr(this)...]
> ...
> callOnMainThread([weakThis = WTFMove(weakThis)...] {
>     if (!weakThis) {
>        callback({ });
>        return;
>     }
>    ...
> });

Because this is a singleton created with a LazyNeverDestroyed.

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:262
>> +        callbackOnExit.release();
> 
> Just my taste but it reads better if callbackOnExit.release(); is called just before moving callback in callOnMainThread lambda.

Sure, not strong feelings about that.

[Sergio Villar Senin]

Created attachment 411853 [details]
Patch

[youenn fablet]

Comment on attachment 411853 [details]
Patch

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

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:235
> +        m_immersiveXRDevices.clear();

We are modifying m_immersiveXRDevices in the work queue but later append to it in the main thread.
This is probably unsafe.
We could probably simply remove this clear here and clear it when appending.
Instead of appending, we could do something like m_immersiveXRDevices = Vector::from(make...).

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254
> +            // device initialization is completed.

This seems a bit weird to do that handling here.
Could we do something like:
- create the device
- have the device an initialise method with a completion handler.
- call callback when initialise completion handler is called?

[Sergio Villar Senin]

(In reply to youenn fablet from comment #13)
> Comment on attachment 411853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411853&action=review
> 
> > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:235
> > +        m_immersiveXRDevices.clear();
> 
> We are modifying m_immersiveXRDevices in the work queue but later append to
> it in the main thread.
> This is probably unsafe.
> We could probably simply remove this clear here and clear it when appending.
> Instead of appending, we could do something like m_immersiveXRDevices =
> Vector::from(make...).

Makes sense.

> > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254
> > +            // device initialization is completed.
> 
> This seems a bit weird to do that handling here.
> Could we do something like:
> - create the device
> - have the device an initialise method with a completion handler.
> - call callback when initialise completion handler is called?

Seems cleaner indeed.

[Sergio Villar Senin]

Created attachment 411967 [details]
Patch

Finally decided to add the callback to the constructor. Creating an init() method was kind of artificial, forced us to expose it just for this case and also required adding type specialization traits to downcast the Devices in the DeviceList to OpenXR devices

[youenn fablet]

Comment on attachment 411967 [details]
Patch

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

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:250
> +            m_immersiveXRDevices.clear();

clear not needed.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255
> +            }));

It might read better like this:
prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) {
    m_immersiveXRDevices = DeviceList::from(WTFMove(device));
   callback(m_immersiveXRDevices);
});
It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly?

[Radar WebKit Bug Importer]

<rdar://problem/70572576>

[Sergio Villar Senin]

Comment on attachment 411967 [details]
Patch

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

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255
>> +            }));
> 
> It might read better like this:
> prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) {
>     m_immersiveXRDevices = DeviceList::from(WTFMove(device));
>    callback(m_immersiveXRDevices);
> });
> It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly?

Not sure what prepareDevice() would look like. Note that we pass a callback to the constructor to ensure that the device has properly initialized everything in the work queue. I don't know how you could achieve that unless
you move all the code in the constructor to that prepareDevice() method. But you cannot do that because the code in the OpenXRDevice constructor initializes several internal attributes of OpenXRDevice. I had previously thought about that but I concluded that it would be better to do everything inside the OpenXRDevice class mainly for encapsulation (although I agree that having a completion callback for a constructor is not very usual).

[Sergio Villar Senin]

Created attachment 412120 [details]
Patch for landing

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #18)
> Comment on attachment 411967 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411967&action=review
> 
> >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255
> >> +            }));
> > 
> > It might read better like this:
> > prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) {
> >     m_immersiveXRDevices = DeviceList::from(WTFMove(device));
> >    callback(m_immersiveXRDevices);
> > });
> > It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly?
> 
> Not sure what prepareDevice() would look like. Note that we pass a callback
> to the constructor to ensure that the device has properly initialized
> everything in the work queue. I don't know how you could achieve that unless
> you move all the code in the constructor to that prepareDevice() method. But
> you cannot do that because the code in the OpenXRDevice constructor
> initializes several internal attributes of OpenXRDevice. I had previously
> thought about that but I concluded that it would be better to do everything
> inside the OpenXRDevice class mainly for encapsulation (although I agree
> that having a completion callback for a constructor is not very usual).

I'd like to hear your opinion anyway about what I wrote above, but note that in the last patch I have just uploaded, the code looks better because I noticed that the OpenXRDevice should call the callback in the main thread because the CompletionHandler was created in the main thread. So the final code looks like:

m_immersiveXRDevices = DeviceList::from(...., [..., callback = WTFMove(callback)]() {
   ASSERT(isMainThread());
   callback(m_immersiveXRDevices);
});

which still requires the completion handler in the constructor but is much more readable.

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #20)
> (In reply to Sergio Villar Senin from comment #18)
> > Comment on attachment 411967 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=411967&action=review
> > 
> > >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255
> > >> +            }));
> > > 
> > > It might read better like this:
> > > prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) {
> > >     m_immersiveXRDevices = DeviceList::from(WTFMove(device));
> > >    callback(m_immersiveXRDevices);
> > > });
> > > It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly?
> > 
> > Not sure what prepareDevice() would look like. Note that we pass a callback
> > to the constructor to ensure that the device has properly initialized
> > everything in the work queue. I don't know how you could achieve that unless
> > you move all the code in the constructor to that prepareDevice() method. But
> > you cannot do that because the code in the OpenXRDevice constructor
> > initializes several internal attributes of OpenXRDevice. I had previously
> > thought about that but I concluded that it would be better to do everything
> > inside the OpenXRDevice class mainly for encapsulation (although I agree
> > that having a completion callback for a constructor is not very usual).
> 
> I'd like to hear your opinion anyway about what I wrote above, but note that
> in the last patch I have just uploaded, the code looks better because I
> noticed that the OpenXRDevice should call the callback in the main thread
> because the CompletionHandler was created in the main thread. So the final
> code looks like:
> 
> m_immersiveXRDevices = DeviceList::from(...., [..., callback =
> WTFMove(callback)]() {
>    ASSERT(isMainThread());
>    callback(m_immersiveXRDevices);
> });
> 
> which still requires the completion handler in the constructor but is much
> more readable.

Youenn ping ^^^

[youenn fablet]

Comment on attachment 412120 [details]
Patch for landing

LGTM.
The callback on constructor feels a bit weird but I guess this is fine.
Ideally, we would not need to hop to background thread, then hop to main thread to create the device, then hop to background thread to initialise it, then hop to main thread to continue processing.
We would do something like: hop to background thread, create/initialize device, hop to main thread with the device (or a cross thread safe device structure), create the vector and continue processing.
Might be worth trying to achieve this in a follow-up patch.

[Sergio Villar Senin]

Committed r269032: <https://trac.webkit.org/changeset/269032>