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

Remove allocations from Marvin.GenerateSeed #32661

Closed
wants to merge 1 commit into from

Conversation

ForNeVeR
Copy link

@ForNeVeR ForNeVeR commented Oct 6, 2018

This removes some unnecessary allocations from Marvin.

@ForNeVeR
Copy link
Author

ForNeVeR commented Oct 6, 2018

The build error is:

10:52:26 /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/Common/src/System/Marvin.cs(114,30):
error CS1503: Argument 1: cannot convert from 'System.Span<byte>' to 'byte[]'
[/mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/Common/tests/Performance/Common.Performance.Tests.csproj]

So, it doesn't know about RandomNumberGenerator.GetBytes(Span<byte>) (added in 2.1 IIRC).

I don't understand how to fix that. Are some performance tests still using old .NET Core version? It it intentional, so my changes cannot be merged? Or should I add a conditional compilation stuff to fix the code for netcoreapp2.0? If so, it seems too much for such a small benefit, so I should probably close the PR.

rng.GetBytes(bytes);
return BitConverter.ToUInt64(bytes, 0);
return MemoryMarshal.Cast<byte, ulong>(bytes)[0];
Copy link
Member

Choose a reason for hiding this comment

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

This is not portable to platforms with strong alignment requirements. This should be MemoryMarshal.Read<ulong>(bytes) instead.

@jkotas
Copy link
Member

jkotas commented Oct 6, 2018

Are some performance tests still using old .NET Core version?

It is not just the performance tests project. The same problem is in other projects that include this file and build against netstandard2.0.

It it intentional, so my changes cannot be merged?

The change would need to be conditional to make it build in all contexts, e.g.

#if netstandard
<original code>
#else
<you new code
#endif

@ForNeVeR
Copy link
Author

ForNeVeR commented Oct 7, 2018

Kindly thanks for your review. I think that conditional compilation is too much for such a small benefit (for single static property evaluation only), so I'll close that PR for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants