Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

ConcurrentQueue 128-byte cache line #22724

Merged
merged 4 commits into from
Sep 17, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1112,10 +1112,10 @@ internal struct Slot

/// <summary>Padded head and tail indices, to avoid false sharing between producers and consumers.</summary>
[DebuggerDisplay("Head = {Head}, Tail = {Tail}")]
[StructLayout(LayoutKind.Explicit, Size = 192)] // padding before/between/after fields based on typical cache line size of 64
[StructLayout(LayoutKind.Explicit, Size = 384)] // padding before/between/after fields based on worst case cache line size of 128
internal struct PaddedHeadAndTail
{
[FieldOffset(64)] public int Head;
[FieldOffset(128)] public int Tail;
[FieldOffset(128)] public int Head;
[FieldOffset(256)] public int Tail;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Xunit.Performance;
using Xunit;

namespace System.Collections.Concurrent.Tests
{
public class Perf_ConcurrentQueueTests
{
/// <summary>
/// Creates a list containing a number of elements equal to the specified size
/// </summary>
[Benchmark]
[InlineData(8, 1000)]
[InlineData(50, 1000)]
public void Enqueue_Dequeue(int threadsCount, int itemsPerThread)
{
Copy link
Member

Choose a reason for hiding this comment

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

Most of the [Benchmark] tests are looping over iterations similarly to the example here: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/performance-tests.md

Probably need to do that here along with calling iteration.StartMeasurement().

var q = new ConcurrentQueue<int>();
Task.WaitAll((from i in Enumerable.Range(0, threadsCount) select Task.Run(() =>
Copy link
Member

Choose a reason for hiding this comment

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

The number of tasks that actually run concurrently will be limited by thread pool heuristics, I suggest using dedicated threads instead or using long-running tasks that don't run on the thread pool:

Task.Factory.StartNew(() =>
{
   ...
}, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kouvel Do you have an example (I don't write much C# core) ? I copied this code from other multi-threaded corefx perf test.

Copy link
Member

Choose a reason for hiding this comment

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

Could follow the general pattern in https://github.com/dotnet/corefx/blob/master/src/System.Collections/tests/Performance/Perf.List.cs. Each iteration could be everything inside the Task.WaitAll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see any thread usage in the example you cite. I'll use dotnet/coreclr#13670 as a sample for thread usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SemaphoreSlimWaitDrainRate() looks like a good sample

Copy link
Member

Choose a reason for hiding this comment

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

Regarding Perf.List.cs, I just meant for that to be an example for a [Benchmark] test.

Many of the tests I wrote in dotnet/coreclr#13670 have a high error margin and they're not all necessarily bad to regress if there's compensation elsewhere so I didn't make them official [Benchmark] tests that are verified by the CI. There's also a ConcurrentQueueThroughput test there.

{
var random = new Random();
Copy link
Member

Choose a reason for hiding this comment

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

May be good to use a specific seed to keep the test deterministic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but It will be hard to keep the test deterministic with many threads contending and sharing the same random number generator.

Copy link
Member

Choose a reason for hiding this comment

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

Yea true, probably won't make much difference

for (int j = 0; j < itemsPerThread; j++)
{
switch (random.Next(2))
{
case 0:
q.Enqueue(random.Next(int.MaxValue));
break;
case 1:
int d;
q.TryDequeue(out d);
break;
}
}
})).ToArray());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<IncludePerformanceTests>true</IncludePerformanceTests>
<ProjectGuid>{13A6510E-E11C-4329-BCDD-4DE561AB6CC0}</ProjectGuid>
<DisableTests Condition="'$(TargetGroup)' == 'uap' AND ('$(ArchGroup)' == 'arm' OR '$(ArchGroup)' == 'arm64')">true</DisableTests>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' " />
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' " />
<ItemGroup Condition="'$(DisableTests)' != 'true'">
<Compile Include="ConcurrentQueuePerfTest.netcoreapp.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the .netcoreapp suffix is necessary, the test itself doesn't require it, it's just not enabled on others for reasons external to this file.

<Compile Include="$(CommonTestPath)\System\PerfUtils.cs">
<Link>Common\System\PerfUtils.cs</Link>
</Compile>
</ItemGroup>
<ItemGroup Condition="'$(DisableTests)' != 'true'">
<ProjectReference Include="$(CommonPath)\..\perf\PerfRunner\PerfRunner.csproj">
<Project>{69e46a6f-9966-45a5-8945-2559fe337827}</Project>
<Name>PerfRunner</Name>
</ProjectReference>
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ static class PaddingHelpers
}

/// <summary>Padding structure used to minimize false sharing in SingleProducerSingleConsumerQueue{T}.</summary>
[StructLayout(LayoutKind.Explicit, Size = PaddingHelpers.CACHE_LINE_SIZE - sizeof(Int32))] // Based on common case of 64-byte cache lines
Copy link
Member

Choose a reason for hiding this comment

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

Why did you delete this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was out of date and inconsistent with the 128 byte value of CACHE_LINE_SIZE.

Copy link
Member

@stephentoub stephentoub Jul 28, 2017

Choose a reason for hiding this comment

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

Ah, I didn't see that the existing value was already set to 128 prior to your change.

[StructLayout(LayoutKind.Explicit, Size = PaddingHelpers.CACHE_LINE_SIZE - sizeof(Int32))]
struct PaddingFor32
{
}
Expand Down