Skip to content

Reduce usage of Win7 and Win8 queues, move to CI and add CI runs in Win arm/arm64 - #35690

Merged
safern merged 6 commits into
masterfrom
HelixQueuesUpdate
May 5, 2020
Merged

Reduce usage of Win7 and Win8 queues, move to CI and add CI runs in Win arm/arm64#35690
safern merged 6 commits into
masterfrom
HelixQueuesUpdate

Conversation

@safern

@safernsafern commented May 1, 2020

Copy link
Copy Markdown
Member

win7 and win8 queues got reduced on number of machines, so I'm reducing the usage by moving them to only run on rolling builds and we can bring them back to PRs whenever we get the needed number of machines.

Also, add libraries test runs for Windows_arm and Windows_arm64 on coreclr in rolling builds.

cc: @MattGal

@ghost

ghost commented May 1, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@safern
safern requested a review from a teamMay 1, 2020 01:11
@safernsafern changed the title Reduce usage of Win7 and Win8 queues, move to CI and add CI runs in Win arm/arm62Reduce usage of Win7 and Win8 queues, move to CI and add CI runs in Win arm/arm64May 1, 2020
@GrabYourPitchforks

Copy link
Copy Markdown
Member

@safern Say that we submit a PR that introduces a Win7-specific regression, but CI passes so we merge it. How do we end up being notified that the rolling build has failed?

@ViktorHofer

Copy link
Copy Markdown
Member

We track failing rolling builds (for whatever reason) in the CI Council as we are scanning all the builds and bucketizing them. If such a regression happens, there'll be an issue created by someone from our team.

@safern

Copy link
Copy Markdown
MemberAuthor

Here is the manual build I queued to test this change since these changes affect rolling builds as well: https://dev.azure.com/dnceng/public/_build/results?buildId=626950&view=results

@safern

Copy link
Copy Markdown
MemberAuthor

I see there was a failure on arm for

System.Tests.ArgIteratorTests.ArgIterator_Throws_PlatformNotSupportedException [FAIL]
Assert.Throws() Failure
Expected: typeof(System.PlatformNotSupportedException)
Actual: typeof(System.ArgumentException): Handle is not initialized.
---- System.ArgumentException : Handle is not initialized.
Stack Trace:
at System.ArgIterator..ctor(IntPtr arglist)
/_/src/coreclr/src/System.Private.CoreLib/src/System/ArgIterator.cs(35,0): at System.ArgIterator..ctor(RuntimeArgumentHandle arglist)
/_/src/libraries/System.Runtime/tests/System/ArgIteratorTests.cs(93,0): at System.Tests.ArgIteratorTests.<>c.<ArgIterator_Throws_PlatformNotSupportedException>b__6_0()
----- Inner Stack Trace -----
at System.ArgIterator..ctor(IntPtr arglist)
/_/src/coreclr/src/System.Private.CoreLib/src/System/ArgIterator.cs(35,0): at System.ArgIterator..ctor(RuntimeArgumentHandle arglist)
/_/src/libraries/System.Runtime/tests/System/ArgIteratorTests.cs(93,0): at System.Tests.ArgIteratorTests.<>c.<ArgIterator_Throws_PlatformNotSupportedException>b__6_0()

I looked at the test and it is conditioned to run only on platforms where ArgIterator is not supported. Arm is one of those archs, but we're not getting a PlatformNotSupportedException.

publicstaticboolIsArgIteratorSupported=>IsMonoRuntime||(IsWindows&&IsNotArmProcess);
publicstaticboolIsArgIteratorNotSupported=>!IsArgIteratorSupported;

It seems like the Arm queue that we use for ARM, is Arm64.

@jkotas do you see anything odd there?

@jkotas

Copy link
Copy Markdown
Member

IsNotArmProcess should be deleted from that condition. The setup for FEATURE_VARARG that ArgIterator falls under is https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/target.h#L10

@safern

Copy link
Copy Markdown
MemberAuthor

Awesome, thanks @jkotas -- I'll include that as part of the PR.

@jashook

Copy link
Copy Markdown
Contributor

This seems correct, we do not support argiterator on windows arm. It should throw a platform not supported error.

@jkotas

Copy link
Copy Markdown
Member

This seems correct, we do not support argiterator on windows arm

So why is FEATURE_VARARG enabled for Windows ARM in the JIT? https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/target.h#L10 Should that be fixed instead?

@jashook

Copy link
Copy Markdown
Contributor

I would need to look into it further, but on first glance I would say yes this define should not be set. I believe we will throw correctly a not supported platform exception on windows arm though. If we do not that should also be addressed.

@jashook

Copy link
Copy Markdown
Contributor

Seems like looking through the test log we are not correctly throwing. This should be a bug.

@jkotas

Copy link
Copy Markdown
Member

This came from your dotnet/coreclr#18373 . The PR description says that varargs are going to be supported on all:

Amd64 Windows
x86 Windows
Arm32 Windows
Arm64 Windows

@safern

Copy link
Copy Markdown
MemberAuthor

So I guess I will open an issue and disable the test for arm, does that sound good?

@safern

Copy link
Copy Markdown
MemberAuthor

@safern

Copy link
Copy Markdown
MemberAuthor

Build failures are known issues. https://dev.azure.com/dnceng/public/_build/results?buildId=630833&view=results

Windows ARM and Windows ARM64 test runs are green. Will go ahead and merge.

@safern
safern merged commit 8b38643 into masterMay 5, 2020
@safern
safern deleted the HelixQueuesUpdate branch May 5, 2020 01:34
@ghostghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@safern@GrabYourPitchforks@ViktorHofer@jkotas@jashook@stephentoub@MattGal@Anipik@Dotnet-GitSync-Bot