Skip to content

HostVisual threading #3567

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Oct 7, 2020
Merged

HostVisual threading #3567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5be1d2f
disconnect HostVisual on its own thread
SamBent Sep 23, 2020
7d20243
repair bad copy/paste
SamBent Sep 23, 2020
e628574
remove AppContext switch
SamBent Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
219 changes: 195 additions & 24 deletions src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,16 @@ internal override void FreeContent(DUCE.Channel channel)

using (CompositionEngineLock.Acquire())
{
DisconnectHostedVisual(
channel,
/* removeChannelFromCollection */ true);
// if there's a pending disconnect, do it now preemptively;
// otherwise do the disconnect the normal way.
// This ensures we do the disconnect before calling base,
// as required.
if (!DoPendingDisconnect(channel))
{
DisconnectHostedVisual(
channel,
/* removeChannelFromCollection */ true);
}
}

base.FreeContent(channel);
Expand Down Expand Up @@ -252,7 +259,7 @@ private void EnsureHostedVisualConnected(DUCE.Channel channel)
//
if (!(channel.IsSynchronous)
&& _target != null
&& !_connectedChannels.Contains(channel))
&& !_connectedChannels.ContainsKey(channel))
{
Debug.Assert(IsOnChannel(channel));

Expand Down Expand Up @@ -323,7 +330,15 @@ private void EnsureHostedVisualConnected(DUCE.Channel channel)
channel);
}

_connectedChannels.Add(channel);
// remember what channel we connected to, and which thread
// did the connection, so that we can disconnect on the
// same thread. Earlier comments imply this is the HostVisual's
// dispatcher thread, which we assert here. Even if it's not,
// the code downstream should work, or at least not crash
// (even if channelDispatcher is set to null).
Dispatcher channelDispatcher = Dispatcher.FromThread(Thread.CurrentThread);
Debug.Assert(channelDispatcher == this.Dispatcher, "HostVisual connecting on a second thread");
_connectedChannels.Add(channel, channelDispatcher);

//
// Indicate that that content composition root has been
Expand Down Expand Up @@ -364,10 +379,11 @@ private void EnsureHostedVisualConnected(DUCE.Channel channel)
/// </summary>
private void DisconnectHostedVisualOnAllChannels()
{
foreach (DUCE.Channel channel in _connectedChannels)
IDictionaryEnumerator ide = _connectedChannels.GetEnumerator() as IDictionaryEnumerator;
while (ide.MoveNext())
{
DisconnectHostedVisual(
channel,
(DUCE.Channel)ide.Key,
/* removeChannelFromCollection */ false);
}

Expand All @@ -382,23 +398,41 @@ private void DisconnectHostedVisual(
DUCE.Channel channel,
bool removeChannelFromCollection)
{
if (_target != null && _connectedChannels.Contains(channel))
Dispatcher channelDispatcher;
if (_target != null && _connectedChannels.TryGetValue(channel, out channelDispatcher))
{
DUCE.CompositionNode.RemoveChild(
_proxy.GetHandle(channel),
_target._contentRoot.GetHandle(channel),
channel
);

//
// Release the targets handle. If we had duplicated the handle,
// then this removes the duplicated handle, otherwise just decrease
// the ref count for VisualTarget.
//

_target._contentRoot.ReleaseOnChannel(channel);

SetFlags(channel, false, VisualProxyFlags.IsContentNodeConnected);
// Adding commands to a channel is not thread-safe,
// we must do the actual work on the same dispatcher thread
// where the connection happened.
if (channelDispatcher != null && channelDispatcher.CheckAccess())
{
Disconnect(channel,
channelDispatcher,
_proxy.GetHandle(channel),
_target._contentRoot.GetHandle(channel),
_target._contentRoot);
}
else
{
// marshal to the right thread
if (channelDispatcher != null)
{
DispatcherOperation op = channelDispatcher.BeginInvoke(
DispatcherPriority.Normal,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use Render priority? I think it's a rendering task.

new DispatcherOperationCallback(DoDisconnectHostedVisual),
channel);

_disconnectData = new DisconnectData(
op: op,
channel: channel,
dispatcher: channelDispatcher,
hostVisual: this,
hostHandle: _proxy.GetHandle(channel),
targetHandle: _target._contentRoot.GetHandle(channel),
contentRoot: _target._contentRoot,
next: _disconnectData);
}
}

if (removeChannelFromCollection)
{
Expand All @@ -407,6 +441,101 @@ private void DisconnectHostedVisual(
}
}

/// <summary>
/// Callback to disconnect on the right thread
/// </summary>
private object DoDisconnectHostedVisual(object arg)
{
using (CompositionEngineLock.Acquire())
{
DoPendingDisconnect((DUCE.Channel)arg);
}

return null;
}

/// <summary>
/// Perform a pending disconnect for the given channel.
/// This method should be called under the CompositionEngineLock,
/// on the thread that owns the channel. It can be called either
/// from the dispatcher callback DoDisconnectHostedVisual or
/// from FreeContent, whichever happens to occur first.
/// </summary>
/// <returns>
/// True if a matching request was found and processed. False if not.
/// </returns>
private bool DoPendingDisconnect(DUCE.Channel channel)
{
DisconnectData disconnectData = _disconnectData;
DisconnectData previous = null;

// search the list for an entry matching the given channel
while (disconnectData != null && (disconnectData.HostVisual != this || disconnectData.Channel != channel))
{
previous = disconnectData;
disconnectData = disconnectData.Next;
}

// if no match found, do nothing
if (disconnectData == null)
{
return false;
}

// remove the matching entry from the list
if (previous == null)
{
_disconnectData = disconnectData.Next;
}
else
{
previous.Next = disconnectData.Next;
}

// cancel the dispatcher callback, (if we're already in it,
// this call is a no-op)
disconnectData.DispatcherOperation.Abort();

// do the actual disconnect
Disconnect(disconnectData.Channel,
disconnectData.ChannelDispatcher,
disconnectData.HostHandle,
disconnectData.TargetHandle,
disconnectData.ContentRoot);

return true;
}

/// <summary>
/// Do the actual work to disconnect the VisualTarget.
/// This is called (on the channel's thread) either from
/// DisconnectHostedVisual or from DoPendingDisconnect,
/// depending on which thread the request arrived on.
/// </summary>
private void Disconnect(DUCE.Channel channel,
Dispatcher channelDispatcher,
DUCE.ResourceHandle hostHandle,
DUCE.ResourceHandle targetHandle,
DUCE.MultiChannelResource contentRoot)
{
channelDispatcher.VerifyAccess();

DUCE.CompositionNode.RemoveChild(
hostHandle,
targetHandle,
channel
);

//
// Release the targets handle. If we had duplicated the handle,
// then this removes the duplicated handle, otherwise just decrease
// the ref count for VisualTarget.
//

contentRoot.ReleaseOnChannel(channel);

SetFlags(channel, false, VisualProxyFlags.IsContentNodeConnected);
}

/// <summary>
/// Invalidate this visual.
Expand Down Expand Up @@ -443,7 +572,49 @@ private void Invalidate()
/// <remarks>
/// This field is free-threaded and should be accessed from under a lock.
/// </remarks>
private List<DUCE.Channel> _connectedChannels = new List<DUCE.Channel>();
private Dictionary<DUCE.Channel, Dispatcher> _connectedChannels = new Dictionary<DUCE.Channel, Dispatcher>();

/// <summary>
/// Data needed to disconnect the visual target.
/// </summary>
/// <remarks>
/// This field is free-threaded and should be accessed from under a lock.
/// It's the head of a singly-linked list of pending disconnect requests,
/// each identified by the channel and HostVisual. In practice, the list
/// is either empty or has only one entry.
/// </remarks>
private static DisconnectData _disconnectData;

private class DisconnectData
{
public DispatcherOperation DispatcherOperation { get; private set; }
public DUCE.Channel Channel { get; private set; }
public Dispatcher ChannelDispatcher { get; private set; }
public HostVisual HostVisual { get; private set; }
public DUCE.ResourceHandle HostHandle { get; private set; }
public DUCE.ResourceHandle TargetHandle { get; private set; }
public DUCE.MultiChannelResource ContentRoot { get; private set; }
public DisconnectData Next { get; set; }

public DisconnectData(DispatcherOperation op,
DUCE.Channel channel,
Dispatcher dispatcher,
HostVisual hostVisual,
DUCE.ResourceHandle hostHandle,
DUCE.ResourceHandle targetHandle,
DUCE.MultiChannelResource contentRoot,
DisconnectData next)
{
DispatcherOperation = op;
Channel = channel;
ChannelDispatcher = dispatcher;
HostVisual = hostVisual;
HostHandle = hostHandle;
TargetHandle = targetHandle;
ContentRoot = contentRoot;
Next = next;
}
}

#endregion Private Fields
}
Expand Down
17 changes: 17 additions & 0 deletions src/Microsoft.DotNet.Wpf/src/WpfGfx/include/exports.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,15 @@ internal struct ChannelSet
/// channel.
/// </summary>
internal sealed partial class Channel
#if ENFORCE_CHANNEL_THREAD_ACCESS
: System.Windows.Threading.DispatcherObject
// "Producer" operations - adding commands et al. - should only be done
// on the thread that created the channel. These operations are on the
// hot path, so we don't add the cost of enforcement. To detect
// violations (which can lead to render-thread failures that
// are very difficult to diagnose), build
// PresentationCore with ENFORCE_CHANNEL_THREAD_ACCESS defined.
#endif
{
/// <summary>
/// Primary channel.
Expand Down Expand Up @@ -768,6 +777,10 @@ unsafe internal void SendCommand(
int cSize,
bool sendInSeparateBatch)
{
#if ENFORCE_CHANNEL_THREAD_ACCESS
VerifyAccess();
#endif

checked
{
Invariant.Assert(pCommandData != (byte*)0 && cSize > 0);
Expand Down Expand Up @@ -808,6 +821,10 @@ unsafe internal void BeginCommand(
int cbSize,
int cbExtra)
{
#if ENFORCE_CHANNEL_THREAD_ACCESS
VerifyAccess();
#endif

checked
{
Invariant.Assert(cbSize > 0);
Expand Down