Skip to content

ReaderWriterLock in PropertyDictionary #4374

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
rainersigwald wants to merge 9 commits into from
Closed

ReaderWriterLock in PropertyDictionary #4374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
60f9d98
ReaderWriterLock in PropertyDictionary
rainersigwald May 14, 2019
ddf872e
Avoid recursive read lock in ToDictionary
rainersigwald May 14, 2019
0de9fc2
fixup! ReaderWriterLock in PropertyDictionary
rainersigwald Jul 10, 2019
4bf72b6
Expand lock in CopyOnReadEnumerable
rainersigwald Jul 10, 2019
7c65b97
Pass PropertyDictionary's ReaderWriterLockSlim to the CopyOnReadEnume…
rainersigwald Jul 10, 2019
15c5052
Merge remote-tracking branch 'Microsoft/master' into propertydictiona…
rainersigwald Jul 10, 2019
f9025d1
Merge remote-tracking branch 'upstream/master' into propertydictionar…
rainersigwald Jul 11, 2019
51d0f5e
Revert "Disable GraphBuild tests to avoid #4368 (#4501)"
rainersigwald Jul 11, 2019
ad7be43
Allow recursion
rainersigwald Jul 11, 2019
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
5 changes: 0 additions & 5 deletions src/Build.UnitTests/BackEnd/BuildManager_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ private void SimpleP2PBuild(BuildParameters buildParameters)
/// A simple successful graph build.
/// </summary>
[Fact]
[ActiveIssue("https://github.com/Microsoft/msbuild/issues/4368")]
public void SimpleGraphBuild()
{
string contents = CleanupFileContents(@"
Expand Down Expand Up @@ -4028,7 +4027,6 @@ handoff logging completion to the BuildManager.
}

[Fact]
[ActiveIssue("https://github.com/Microsoft/msbuild/issues/4368")]
public void GraphBuildValid()
{
string project1 = _env.CreateFile(".proj").Path;
Expand Down Expand Up @@ -4069,7 +4067,6 @@ public void GraphBuildValid()
}

[Fact]
[ActiveIssue("https://github.com/Microsoft/msbuild/issues/4368")]
public void GraphBuildInvalid()
{
string project1 = _env.CreateFile(".proj").Path;
Expand Down Expand Up @@ -4101,7 +4098,6 @@ public void GraphBuildInvalid()
}

[Fact]
[ActiveIssue("https://github.com/Microsoft/msbuild/issues/4368")]
public void GraphBuildFail()
{
string project1 = _env.CreateFile(".proj").Path;
Expand Down Expand Up @@ -4144,7 +4140,6 @@ public void GraphBuildFail()
}

[Fact]
[ActiveIssue("https://github.com/Microsoft/msbuild/issues/4368")]
public void GraphBuildCircular()
{
string project1 = _env.CreateFile(".proj").Path;
Expand Down
1 change: 0 additions & 1 deletion src/Build.UnitTests/Graph/ProjectGraph_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

namespace Microsoft.Build.Experimental.Graph.UnitTests
{
[ActiveIssue("https://github.com/Microsoft/msbuild/issues/4368")]
public class ProjectGraphTests : IDisposable
{
private TestEnvironment _env;
Expand Down
40 changes: 39 additions & 1 deletion src/Build/Collections/CopyOnReadEnumerable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections;
using System.Collections.Generic;
using System.Threading;
using Microsoft.Build.Shared;

namespace Microsoft.Build.Collections
Expand All @@ -28,6 +29,8 @@ internal class CopyOnReadEnumerable<T> : IEnumerable<T>
/// </summary>
private readonly object _syncRoot;
Copy link
Member

@danmoseley danmoseley Jul 10, 2019

Choose a reason for hiding this comment

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

You can eliminate this field now and use the RW lock? It doesn't matter what the object is so long as it's readonly and private. That will claw back the size increase in this object form adding the RW lock field.

Copy link
Member

@danmoseley danmoseley Jul 10, 2019

Choose a reason for hiding this comment

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

Disregard, I didn't view the full diff. You still use that syncroot in another path.


private readonly ReaderWriterLockSlim _readerWriterLockSlim;

/// <summary>
/// Constructor.
/// </summary>
Expand All @@ -42,6 +45,20 @@ public CopyOnReadEnumerable(IEnumerable<T> backingEnumerable, object syncRoot)
_syncRoot = syncRoot;
}

/// <summary>
/// Constructor.
/// </summary>
/// <param name="backingEnumerable">The collection which serves as a source for enumeration.</param>
/// <param name="readerWriterLock">The object used to synchronize access for copying.</param>
public CopyOnReadEnumerable(IEnumerable<T> backingEnumerable, ReaderWriterLockSlim readerWriterLockSlim)
{
ErrorUtilities.VerifyThrowArgumentNull(backingEnumerable, nameof(backingEnumerable));
ErrorUtilities.VerifyThrowArgumentNull(readerWriterLockSlim, nameof(readerWriterLockSlim));

_backingEnumerable = backingEnumerable;
_readerWriterLockSlim = readerWriterLockSlim;
}

#region IEnumerable<T> Members

/// <summary>
Expand All @@ -62,8 +79,20 @@ public IEnumerator<T> GetEnumerator()

bool isCloneable = false;
bool checkForCloneable = true;
lock (_syncRoot)

object lockObject = _syncRoot;
bool lockWasTaken = false;
try
{
if (lockObject != null)
{
Monitor.Enter(lockObject, ref lockWasTaken);
}
else
{
_readerWriterLockSlim.EnterReadLock();
}

foreach (T item in _backingEnumerable)
{
if (checkForCloneable)
Expand All @@ -80,6 +109,15 @@ public IEnumerator<T> GetEnumerator()
list.Add(copiedItem);
}
}
finally
{
if (lockWasTaken)
{
Monitor.Exit(lockObject);
}

_readerWriterLockSlim?.ExitReadLock();
}

return list.GetEnumerator();
}
Expand Down
Loading