-
Notifications
You must be signed in to change notification settings - Fork 4.9k
ConcurrentQueue 128-byte cache line #22724
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
{ | ||
var q = new ConcurrentQueue<int>(); | ||
Task.WaitAll((from i in Enumerable.Range(0, threadsCount) select Task.Run(() => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you delete this comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
{ | ||
} | ||
|
There was a problem hiding this comment.
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().