Dont run the Span CopyToLargeSizeTest for 32-bit process.#18563
Dont run the Span CopyToLargeSizeTest for 32-bit process.#18563danmoseley merged 1 commit intodotnet:masterfrom
Conversation
|
@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! |
|
@dotnet-bot test Innerloop Windows_NT Debug x86 Build and Test |
|
@dotnet-bot test outerloop Windows Nano 2016 Debug |
|
@dotnet-bot test outerloop netcoreapp Windows 10 Debug |
| public static void CopyToLargeSizeTest(long bufferSize) | ||
| { | ||
| // If this test is run in a 32-bit process, the large allocation will fail. | ||
| if (Unsafe.SizeOf<IntPtr>() != sizeof(long)) |
There was a problem hiding this comment.
does !Environment.Is64BitProcess not work?
There was a problem hiding this comment.
Or just use IntPtr.Size == 8.
There was a problem hiding this comment.
I followed the examples here:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Span/Overflow.cs#L25
Or just use
IntPtr.Size == 8.
I could do:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Span/Clear.cs#L233
What is the advantage of doing that? What is the difference between Unsafe.SizeOf<>() and sizeof()?
There was a problem hiding this comment.
It's just more convoluted, and involves unnecessary usage of Unsafe. It also relies on an unclear relationship between the size of IntPtr and the size of Int64.
I think we use IntPtr.Size == 8 in other parts of corefx, but if this pattern is more consistent with the Span tests, then it's okay.
There was a problem hiding this comment.
What is the difference between Unsafe.SizeOf<>() and sizeof()?
sizeof(IntPtr) can only be called from an "unsafe" context. Unsafe.SizeOf<T>() can be called anywhere. They are otherwise identical. IntPtr.Size will also give you the same value.
There was a problem hiding this comment.
It also relies on an unclear relationship between the size of IntPtr and the size of Int64.
Is the relationship unclear? I thought IntPtr can only be one of 32-bit or 64-bit.
There was a problem hiding this comment.
I guess it's a subtle distinction. What you want to know is whether the pointer size is 8 bytes or larger, because you are making a large memory allocation which might fail otherwise. Comparing it to the size of another type feels a little roundabout to me, when you really just want to know if it is 8 bytes or larger.
There was a problem hiding this comment.
It's probably obvious but Environment.Is64BitProcess literally returns IntPtr.Size == 8. I don't know why not to use it but it's boviously not a big deal :)
|
@dotnet-bot test outerloop Windows Nano 2016 Debug |
|
@danmosemsft
The outerloop tests only run for x64. |
|
We don't run x86 at all on outer loop, which is not great. I'll follow up on that offline. Meantime you will have to run locally. |
|
ie add /p:outerloop=true |
|
What is the resolution for this CI failure (if one exists)?
|
How did we catch this issue then? https://github.com/dotnet/corefx/issues/18540 |
|
@dotnet-bot test outerloop Windows Nano 2016 Debug |
|
Yeah offline @weshaggard pointed out they just aren't labeled right on the main page of MC. If you drill down, they are x86. |
I ran the tests locally using:
=== TEST EXECUTION SUMMARY === There are 6 outerloop tests that "run" and 5 of them just return since they get skipped for x86 architecture (as expected). |
|
@shiftylogic, can this be merged? |
|
I'd say yes. The failing Outerloop test is not from this change. The change is benign other than disabling the test for 32-bit. |
|
@mmitche is |
|
@danmosemsft It looks like Azure has stopped deploying the VM image correctly (it's timing out). I'm contacting support. |
|
@danmosemsft I think this might be just that this was an old TP5 2016 image that expired. Looking into whether I can just move this forward to another image. |
Fixes #18540
cc @shiftylogic, @WinCPP