Bug 217752 - [WebXR] Move OpenXR calls off the main thread
Summary: [WebXR] Move OpenXR calls off the main thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks: 208988 216925
  Show dependency treegraph
 
Reported: 2020-10-15 07:02 PDT by Sergio Villar Senin
Modified: 2021-01-11 07:50 PST (History)
7 users (show)

See Also:


Attachments
Patch (17.03 KB, patch)
2020-10-15 07:42 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (17.04 KB, patch)
2020-10-15 08:16 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (15.98 KB, patch)
2020-10-19 08:17 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (16.33 KB, patch)
2020-10-20 04:59 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2020-10-21 02:38 PDT, Sergio Villar Senin
youennf: review+
Details | Formatted Diff | Diff
Patch for landing (16.27 KB, patch)
2020-10-22 12:02 PDT, Sergio Villar Senin
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2020-10-15 07:02:50 PDT
[WebXR] Move OpenXR calls off the main thread
Comment 1 Sergio Villar Senin 2020-10-15 07:42:43 PDT
Created attachment 411442 [details]
Patch
Comment 2 Sergio Villar Senin 2020-10-15 08:16:22 PDT
Created attachment 411443 [details]
Patch
Comment 3 Sergio Villar Senin 2020-10-16 15:02:49 PDT
Gentle ping for reviewers
Comment 4 Zan Dobersek 2020-10-17 01:46:28 PDT
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).
Comment 5 Sergio Villar Senin 2020-10-17 02:26:23 PDT
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.
Comment 6 youenn fablet 2020-10-19 02:43:08 PDT
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.
Comment 7 Sergio Villar Senin 2020-10-19 08:17:08 PDT
Created attachment 411747 [details]
Patch
Comment 8 Sam Weinig 2020-10-19 08:44:28 PDT
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?
Comment 9 Sergio Villar Senin 2020-10-19 12:25:47 PDT
(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/
Comment 10 youenn fablet 2020-10-20 00:52:33 PDT
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.
Comment 11 Sergio Villar Senin 2020-10-20 03:50:10 PDT
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.
Comment 12 Sergio Villar Senin 2020-10-20 04:59:14 PDT
Created attachment 411853 [details]
Patch
Comment 13 youenn fablet 2020-10-20 05:17:20 PDT
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?
Comment 14 Sergio Villar Senin 2020-10-20 06:52:47 PDT
(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.
Comment 15 Sergio Villar Senin 2020-10-21 02:38:59 PDT
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
Comment 16 youenn fablet 2020-10-21 06:18:37 PDT
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?
Comment 17 Radar WebKit Bug Importer 2020-10-22 07:03:20 PDT
<rdar://problem/70572576>
Comment 18 Sergio Villar Senin 2020-10-22 11:55:41 PDT
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).
Comment 19 Sergio Villar Senin 2020-10-22 12:02:05 PDT
Created attachment 412120 [details]
Patch for landing
Comment 20 Sergio Villar Senin 2020-10-22 12:06:42 PDT
(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.
Comment 21 Sergio Villar Senin 2020-10-27 03:08:57 PDT
(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 ^^^
Comment 22 youenn fablet 2020-10-27 03:16:58 PDT
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.
Comment 23 Sergio Villar Senin 2020-10-27 05:31:02 PDT
Committed r269032: <https://trac.webkit.org/changeset/269032>