ConcurrentQueue 128-byte cache line #22724
Conversation
|
@dotnet/arm64-contrib @stephentoub @kouvel PTAL |
|
Performance tests/results to justify the change? |
|
No only implicit evidence that these hot cache lines impacted performance for 64-byte cache lines and were therefore worthy of optimizing. There is no reason to believe the same is not true for 128-byte line caches. I will measure performance impact of the equivalent change in CoreCLR on plaintext. Since the CoreCLR code is a copy of this code, I wanted to keep them in sync. |
|
I understand, but this change isn't free; it's increasing the size of each segment by 192 bytes, even on (the vast majority?) of architectures with 64-byte cache lines. That may not be a big deal (after all, hopefully we're successfully reusing segments and each queue has a minimal number of them, and the ratio of that extra size to the size of the segment decreases as the segment sizes grow), but it's at least worth verifying is worthwhile. How common are 128-byte cache lines? |
| @@ -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.
Why did you delete this comment?
There was a problem hiding this comment.
I was out of date and inconsistent with the 128 byte value of CACHE_LINE_SIZE.
There was a problem hiding this comment.
Ah, I didn't see that the existing value was already set to 128 prior to your change.
I am not sure. Many Qualcomm mobile arm32 & arm64 use 128-byte cache lines. All Qualcomm Datacenter parts currently use 128-byte cache lines, I believe PowerPC also used. In CoreCLR mscorlib there is |
Ok, thanks.
There's a single build of System.Collections.Concurrent.dll, regardless of OS, architecture, etc. We'd need to change that, which is quite expensive from a build and deployment perspective. And even if we did, to my knowledge we don't have any way of saying "use this particular build on machines with this cache line size"... we'd need to assume a particular size based on a particular architecture. Doesn't seem worthwhile. I'd love to see some data that validates this change is worthwhile on 128-byte cache lines. It very likely is, but any time we add cost, we should validate it's for a good reason. Assuming it is, I'm fine with the change. |
|
Every time I see changes like this I think of the promise of .Net when it was introduced to optimize for the platform the program is running on. Given this, it seem odd to hard-code the cache line size. Can it be done at runtime, or better yet, by the jitter? https://stackoverflow.com/questions/794632/programmatically-get-the-cache-line-size |
|
Would it make sense to move all of the concurrent collections to CoreLib? All of them are used by CoreLib and it seems redundant to have a copy there and here. Since CoreLib is already built for different architectures, it would make it easy to special-case this for the architecture. |
I'm not clear how special-casing for the arch is valuable here, as the cache line size isn't tied to an architecture. Am I missing something? |
ConcurrentBag isn't used, is it? ConcurrentStack is used only by PinnableBufferCache, and honestly I'm not sure that's the right choice for it, given CS's allocation profile. Separately we should revisit that. ConcurrentDictionary and ConcurrentQueue are both used, though only a relatively small portion of their surface area. That said, I don't have a strong opinion about where the types live; we could certainly look at moving them if it was worthwhile. |
I would, personally, rather see a general way to say: Automatic Layout with Padding based on Cache Line size. Users outside of CoreFX/CoreCLR should be able to implement their own "optimized" data structures where the can force any alignment and do "intelligent" things, like padding structures based on the cache line size/etc. |
At one point we investigated adding a |
|
@stephentoub ARM64 Ubuntu perf data: For plaintext, the CoreCLR equivalent of this change results in a 17% improvement on my tip. |
@stephentoub, is there an existing issue for this? If not, I'm going to log one 😄 |
I don't believe so. |
Ok, thanks. Let's go with this for now, and hopefully the extra memory consumption will be inconsequential. |
|
@dotnet-bot test Packaging All Configurations x64 Debug Build please |
Since this is a performance related PR affecting common code, it would be good to understand performance impact on other platforms. @sdmaclea Do you have any data that show the change does not regress performance on x64? |
It will regress, at least in terms of memory consumption, with every segment being larger than it used to be. It's a good point that in theory it could regress throughput as well (due to needing to load more memory), though I doubt it would in any meaningful way. |
Understood. What I am saying is that for performance related PRs, they need to be data driven - sometimes what we expect to see is not what we get. |
@hanblee No measurable impact expected. No data collected. |
|
What is status of this PR? No update for last 2+ weeks. I would love to see perf tests checked in along side this change, so that we can independently verify the improvement and no impact on other platforms. Is that reasonable ask? |
In general, we expect every submitter of a PR (MS, member, non-member) to see it through end-to-end, on all affected platforms, including verification that there is no regression when there are concerns. Functional is covered by CI legs. Perf and other concerns (e.g. stress) are manual by submitter (in a form which can be potentially verified by others if desired). |
|
@karelz > In general, we expect every submitter of a PR (MS, member, non-member) to see it through end-to-end, on all affected platforms, LOL. you must be joking. |
Not really. Why do you think so? We always held all developers responsible for their code changes end-to-end and from all aspects (that holds for open-source and closed-source .NET source code, even prior to any OSS efforts on our side). Such end-to-end means that the author has to make sure all tests pass. If there are unrelated test failures (proved to be unrelated noise), we expect the author to seek help/file bugs (sometimes other engineers jump in when they notice, but it is not a service we guarantee). It is a bar we hold for all our Microsoft engineers and I don't see any reason why we should soften it significantly for anyone else. Of course, we try to help more external contributors, or new team members, provide guidance and make sure everyone is successful, but we do not have a magic "test team" which would go and make sure someone's changes are ok prior to checkin -- code reviews are supposed to call out concerns and it is up to author to address them. It all feels very natural to me, so I am curious if you have different view point. |
|
@karelz I agree in principle to most of what you are saying. The comment came off as harsh and off topic. You had requested a micro-benchmark in an earlier comment because you said it would allow others to independently verify the performance impact of this change. When you asked for next steps, I assumed this verification was a step you intended. I was simply saying the test was ready to be used/tried. So given your comment, the the next step is a code review. I will also in parallel try to run the new perf test on arm64. |
|
@dotnet-bot help |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
@karelz I see there are no perf tests in CI. Looks like this is issue #19200. If you want to include in dev expectations, then #19200 is critical. |
|
Sorry for the delay, I'll take a look shortly |
| [InlineData(8, 1000)] | ||
| [InlineData(50, 1000)] | ||
| public void Enqueue_Dequeue(int threadsCount, int itemsPerThread) | ||
| { |
There was a problem hiding this comment.
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().
| 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.
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.
@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.
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.
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.
SemaphoreSlimWaitDrainRate() looks like a good sample
There was a problem hiding this comment.
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 q = new ConcurrentQueue<int>(); | ||
| Task.WaitAll((from i in Enumerable.Range(0, threadsCount) select Task.Run(() => | ||
| { | ||
| var random = new Random(); |
There was a problem hiding this comment.
May be good to use a specific seed to keep the test deterministic
There was a problem hiding this comment.
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.
Yea true, probably won't make much difference
| <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.
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.
|
On x64, I'm seeing no change in perf on your test. On my ConcurrentQueueThroughput test in dotnet/coreclr#13670, I'm seeing an improvement: |
|
@kouvel, thanks for checking. Presumably your machine has 64-byte cache lines? I'd think the best we could hope for is that this doesn't regress perf in that case (so I'm a little surprised there was a small bump in one of the tests... maybe just noise?) |
Not sure if you mean any of my previous answers. If yes, I apologize. Text is not the best way to provide all context, doubts and meanings :(.
I see. Sorry for the confusion - my original ask (more a suggestion) for micro-benchmark was targeted as "having evidence" and giving anyone chance to review the micro-benchmark itself (incl. its completeness / coverage) and also giving anyone the option to replicate same results if that person chooses to do so.
If the code could affect perf also on x86 and amd64 (in a different way than arm32 and arm64), we will need verification on those platforms as well prior to checkin. I will let final decision on the reviewers of the change.
This is more complicated than one would like to. Let me try to explain: |
|
@stephentoub, I tried on two different processors, both with 64-byte cache line size and I'm consistently seeing a 5-8% improvement on both. |
Any theories as to why? |
|
I can't think of a reason, maybe the extra separation is allowing it to somehow/accidentally parallelize the memory accesses better |
Ok, thanks. Given that this improves things for a known reason on some systems, and at least doesn't regress throughput elsewhere (it'll regress memory overheads, but it shouldn't be significant compared to the arrays being used), so I'm fine with it. |
|
@dotnet-bot help |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
I did revise the draft perf test per @kouvel suggestion. However I removed it based on:
I left it in the PR history in case someone wants to revive |
|
LGTM, thanks! |
* Microsoft.ServiceModel.Syndication skeleton project * Adding the existing classes of SyndicationFeed from .net fx * Added the needed references to get the code to compile * Changed some namespaces * Fixed errors when reading feeds and replaced some buffers * Cleaning code for PR * Deleted some unused files * Added the posibility that the user creates his own date parser when reading a feed from outside, also as part of our default date parser, if it can't parse the date it will just assign a default date to avoid the crash * Added correct testnames and Copyright * Initial changes to add custom parsers * Added more delegates as parsers * Added custom parser delegates * Save changes * Initial SyndicationFeed * Removed the dependence of the old SR class * Cleaned the code, deleted Diagnostics, fixed throwing resources * Cleaned code from most of the unnecesary comments * Formatted the code with CodeFormatter Tool * Moved the call of itemParser to be called with each item * Test using custom parsers * Fixed image issue where image tag was using original title and url, for RSS formatting Image issue fixed and added some tests * Test clase jorge * Save changes with Jorge cass * Initial Jorge * saving changes * Save changes * Fixed image and items issue * Fixed disjoint items in atom * Run codeFormatter * Adding parsing to optional elements int the spec added unittesting * Added Icon parsing to Atom10 feedFormatter. Added unit test * Adding Async Writing RssFormater writes new optional spec elements * Added Icon writing on Arom Writer * Fixed some warnings * Improved custom parsing for RSS * Added custom parsing for Atom feed formatter, added test * added nameof() to all exceptions when needed. * Adding Extension Methods for XmlReader * Fixing code for PR * Fixing code for PR * Added check for skip days to allow only accepted days. * Improved flexibility for Default dateparser * Add wrong dates example * Fixing code review * Fixed warnings on some unawaited methods. * Added async extension methods for XmlReader * Add XmlWriterWrapper method extensions * Changed ReadCategoryFromAsync to return a SyndicationCategory * Fixed sync method exposed GetReader. * Edited XmlReaderWrapper, moved methods to extension methods. * Removed unnecesary variables from Wrappers. * Fixed bug from ServiceModel.Syndication * Make BigInteger ctor opt path for == int.MinValue reachable. The BigInteger(ReadOnlySpan<byte> value) ctor contains an optimised path for value being larger than 4 bytes and the result being equal to int.MinValue. However this path is inside a test that precludes that value, so can never be reached. Restructure so that the path is reachable. * Fix ServiceModel.SyndicationFeed project dependencies - Change M.SM.SyndicationFeed to be a .NET Standard 2.0 library - Change tests to use official .NET Core 2.0 release from preview * Add btrfs and other missing file system types (dotnet#24102) * Uppercase * Add more filesystems from stat.c. Change to stat names where possible * Add some more local friendly names * Add some more remote file types from stat.c * Add entry to switch present in enum * Build errors * Remove Fixed entries that should be RAM * comment * Change case of 0X to 0x * Move cramfs to Fixed * ConcurrentQueue 128-byte cache line (dotnet#22724) ConcurrentQueue 128-byte cache line * Add warning by default in SGEN (dotnet#24054) * Output warning by default if run the tool directly without /quiet parameter. * add quiet parameter in the command. * fix parameter error. * Update the warning. * Add the target to copy the serializer to publish folder. (dotnet#24096) * Wrap cert callback leaked exceptions in WinHttpHandler (dotnet#24107) Similar to the fix done for CurlHandler (dotnet#21938), fix WinHttpHandler so that leaked exceptions from the user-provided certificate callback are properly wrapped. The ManagedHandler still needs to be fixed since it is not wrapping properly. Contributes to #21904 * Ensure ProcessModule for main executable is first in modules list (dotnet#24106) * Disable NegotiateStreamTest fixture for distros without working Kerberos (dotnet#24098) Disable NegotiateStreamTest fixture entirely because setup-kdc.sh is broken on some distros * Expose and add tests for Guid.ctor(Span)/TryWriteBytes/TryFormat * Address remaining PR feedback * #24112 Replaced documentation summary of TryPop with text similar to TryDequeue. (dotnet#24113) * Wrap exceptions from ManagedHandler's server validation callback (dotnet#24111) To match other handlers' behaviors. * Fix libgdiplus function loading on OSX. * Prevent ServiceControllerTests from dispose when already disposed (dotnet#24042) * Disable ServiceProcessTest that has been failing on CI and official builds * Prevent dispose from been called when already disposed * Add logging to repro if RemoveService is been called twice on the same ServiceController * Data-annotations length fix (dotnet#24101) * Updated in MinLengthAttribute and MaxLengthAttribute to support ICollection<T> * Added tests * Fixed typo * Trying to address two failing checks: - Linux x64 Release Build - UWP CoreCLR x64 Debug Build * Implemented changes requested in review - Extracted Count checking to an external helper to obey DRY - Removed dependency of ICollection<T> and changed to simple reflection Count property lookup * Added requested tests * Added catch for MissingMetadataException. * Extracted code from try-catch. * Added comment as requested. * Typo correction * Remove System.Drawing.Common from the netfx compat package (temporary). * Updating corefx to use new roslyn compiler (dotnet#24076) * Updating corefx to use new roslyn compiler * Updating to new version of the compiler and use the switch so tests pass on desktop * Fix System.Reflection.Metadata tests * Stop running Math.Clamp tests on UAP as API is not there (dotnet#24125) * Stop running Math.Clamp tests on UAP as API is not there * Disable only for AOT * Fix instantiating PrincipleContext with Domain type (dotnet#24122) * Fix instantiating PrincipleContext with Domain type * Enhancement * Disable Test_Write_Metric_eventListener on UWP. (dotnet#24127) * Revert "Remove System.Drawing.Common from the netfx compat package (temporary)." This reverts commit f6b0fbd. * Update BuildTools, CoreClr, CoreFx, CoreSetup, Standard to prerelease-02019-01, preview1-25718-02, preview1-25718-03, preview1-25717-02, preview1-25718-01, respectively (dotnet#24075) * Fixed compile warning/error on FreeBSD (dotnet#24141) * Marking {ReadOnly}Span as readonly structs (dotnet#23908) * Marking {ReadOnly}Span as readonly structs, fixing issue #23809 * Adding readonly attributes to reference assemblies. * Using "readonly ref" keyword instead of attributes. * Adding a LangVersion 7.2 property * System.Drawing: Throw ArgumentNullException on Unix as well (dotnet#24140) * Throw ArgumentNullException when stream is null * Throw ArgumentNullException when stream is null * Remove invalid NameResolution tests (dotnet#24147) The Dns_GetHostEntryAsync_* are fundamentially invalid because it's possible for the broadcast address, 255.255.255.255, to have an DNS mapping via manually modifying the hosts file. This was actually happening on Mac systems as well as an virtual environment running on top of that (i.e. Windows on Parallels). Ref: https://github.com/dotnet/corefx/issues/23992#issuecomment-330250642 Contributes to #23992 * Bump system.drawing.common.testdata to 1.0.6 * Update BuildTools to prerelease-02019-02 (dotnet#24146) * Fix path to test data * Fix whitespace * Add GraphicsTests based on Mono's test suite. * Consolidate more code in the "System.Drawing" namespace. * Remove all remaining Win32 codepaths from the mono codebase. All of this code now implicitly assumes that it will be run on a Unix platform. * Consolidate the rest of the gdipFunctions.cs file into Gdip.cs and GdipNative.Unix.cs * Consolidate the GraphicsUnit and ImageType enumerations -- they were duplicated. * Remove the mono Status enum and use the Windows constants instead in all Unix code. * Move all files into the regular directory structure. Suffix them with ".Unix" and ".Windows" when there are collisions. * Tiny bit of code cleanup * Add conditionals for recent versions of mono * Remove duplicate tests. * Fix multiplying TextureBrush with a disposed matrix on Unix (dotnet#24109) * Fix multiplying TextureBrush with a disposed matrix on Unix * Enable another passing test * Fix accidentally committed file * Add an error code fixup to Bitmap.Unix.cs to match Windows. * Bump System.Drawing.Common.TestData to 1.0.6 (dotnet#24149) * Bump system.drawing.common.testdata to 1.0.6 * Fix path to test data * Fix whitespace * Cleanup - Add/simplify using statements of disposable resources (Graphics, Bitmap) * Validate HatchStyle passed to HatchBrush ctor * Delete accidentally duplicated HatchBrush tests in LinearGradientBrushTests * Remove references to historical Mono bug IDs * Renable some already passing tests * Remove Thread.Sleep workaround * Update BuildTools to prerelease-02020-01 (dotnet#24172) * Add thread-local based switch to opt-in to ManagedHandler * Address PR feedback * PR feedback * Fix memory map imports (dotnet#24176) * Fix memory map imports Imports lost the last error attribute. Add it back and change the results to be the more correct "bool". Tweak the usage based on the new return type. #24159 * Move spin wait * Remove unused FEATURE_RANDOMIZED_STRING_HASHING (dotnet#24178) * Adding System.Data.Odbc package and including in metapackage * Fix MultiplyTransform with a disposed brush * Fix for 2nd Connection hangs in Mirroring Environment (dotnet#24181) * Switch tests to use thread-local switch for ManagedHandler And as a result re-enable parallelism of the test suite, which on my machine reduces the running time of the outerloop tests from 150s to 45s. * Update CoreClr, CoreSetup, ProjectNTfs, ProjectNTfsTestILC, Standard to preview1-25720-03, preview1-25719-04, beta-25721-01, beta-25721-01, preview1-25721-01, respectively * CoreFx #22406 Span based APIs - Text Reader Writer (dotnet#23786) * [Drawing] Move remaining "Unix" files into regular directory structure * As part of this, the "Windows" version of ImageFormat.cs is now being used everywhere. This file just contains a bunch of GUID's that are not platform-specific in any way. * Change AsSpan to property Span and rename AsMemory to Memory * Add Metafile and MetaHeader tests based on Mono's System.Drawing unit tests * Update project file * MetafileTests: Remove duplicates, and re-enable tests on Unix which are now working. Delete duplicate MetaHeadertests * Rationalize using statements * Update project file * Fix Metafile exception behavior * Simplify tests, remove duplicate test * Don't catch an ArgumentException just to get a NullReferenceException instead. * Assert.False(Object.ReferenceEquals => Assert.NotSame * PR feedback * Remove duplicate tests, fix typo * Remove X11 dependency for tests which are enabled on Unix. * Workaround libgdiplus glitches Don't test metafileHeader.Bounds when using libgdiplus. Don't test metafile.GetBounds on Unix, force MetafileHeader.Bounds to return an empty rectangle when MetafileSize == 0 Don't validate graphicsUnit on Unix. * Move Syndication to root/src * Disable Build of S.SM.Syndication. * Increase file descriptor limit in S.N.Http tests on macOS * Expose/test String.Create span-based method (dotnet#23872) * Expose/test String.Create span-based method * Address PR feedback * Update build to clang/llvm 3.9 (dotnet#24177) Update scripts, docs and build pipeline docker images to clang/llvm/lldb 3.9 * Update ProjectNTfs, ProjectNTfsTestILC, Standard to beta-25722-00, beta-25722-00, preview1-25722-01, respectively (dotnet#24207) * Remove the line that will copy the generated serializer to the pack. (dotnet#24199) * Revert "Update build to clang/llvm 3.9 (dotnet#24177)" This reverts commit 21e008a. * Remove stale SetStateMachine call in test * Ssl Stream Async Write (dotnet#23715) * Change from APM to Async/Await for write side * Added nameof * Reacting to review * Reacting to review * SSLStream : Fixed spelling mistake in file name (extra a) (dotnet#24221) * Fixed spelling mistake in file name * Fix csproj * Update ProjectNTfs, ProjectNTfsTestILC to beta-25723-00, beta-25723-00, respectively (dotnet#24222)
ConcurrentQueue 128-byte cache line Commit migrated from dotnet/corefx@9c468a0
No description provided.