Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pipeline setup for jit offbydefault extra testing #33709

Merged
merged 7 commits into from
Mar 19, 2020

Conversation

AndyAyersMS
Copy link
Member

Create a pipeline to test functionality in the jit that is not (yet)
enabled by default:

  • object stack allocation
  • eh write through
  • on stack replacement (+ osr stress)
  • guarded devirtualization

Currently just x64, pri1 tests.

Create a pipeline to test functionality in the jit that is not (yet)
enabled by default:
* object stack allocation
* eh write through
* on stack replacement (+ osr stress)
* guarded devirtualization

Currently just x64, pri1 tests.
@AndyAyersMS
Copy link
Member Author

@dotnet/jit-contrib PTAL

Trial run here (has some failures we'll need to sort through).

@AndyAyersMS
Copy link
Member Author

Cancelled run was against the staging branch -- I was wanted to see if the OSX dependence had been properly removed.

Not clear why it shows up here. I suppose whatever collates testing status is matching on commit.

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

LGTM expect small nits

@@ -126,6 +133,11 @@
<TestEnvironment Include="gcstress0xc_jitstress1" GCStress="0xC" JitStress="1" />
<TestEnvironment Include="gcstress0xc_jitstress2" GCStress="0xC" JitStress="2" />
<TestEnvironment Include="gcstress0xc_jitminopts_heapverify1" GCStress="0xC" JITMinOpts="1" HeapVerify="1" />
<TestEnvironment Include="jitosr" TC_OnStackReplacment="1" TC_QuickJitForLoops="1" TieredCompilation="1"/>
Copy link
Contributor

@echesakov echesakov Mar 18, 2020

Choose a reason for hiding this comment

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

Nit: for consistency can you please put a space between " and />.

@@ -378,6 +378,13 @@ jobs:
scenarios:
- jitelthookenabled
- jitelthookenabled_tiered
${{ if in(parameters.testGroup, 'jit-offbydefault-extra') }}:
Copy link
Contributor

@echesakov echesakov Mar 18, 2020

Choose a reason for hiding this comment

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

Is there any alternatives for naming this pipeline other than offbydefault?

jitexperimentalfeatures? or just jitexperimental

My first thoughts when I read offbydefault were that this pipeline is off by default. But then I saw a schedule trigger and realized that this is about the jit features.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to rename if it reduces confusion -- @dotnet/jit-contrib any other thoughts/suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

I like "jit-experimental". I don't think you need "-extra" (We could save that for a version of this that also does JitStress modes, perhaps.)


schedules:
- cron: "0 10 * * 6,0"
displayName: Sat and Sun at 2:00 AM (UTC-8:00)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any other pipelines running at the same time that would compete with this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

This kicks off at the same time as the other weekend runs, I can certainly change it...

@AndyAyersMS
Copy link
Member Author

Test environment syntax was off in a few other places as well.

@echesakov
Copy link
Contributor

Test environment syntax was off in a few other places as well.

Thank you for fixing this!

- cron: "0 10 * * 6,0"
displayName: Sat and Sun at 2:00 AM (UTC-8:00)
- cron: "0 22 * * 0"
displayName: Sun at 2:00 PM (UTC-8:00)
Copy link
Member

Choose a reason for hiding this comment

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

IMO, it's useful for it to run both Saturday and Sunday. Not because we need both runs, but because you're less likely for the weekly testing to be completely torpedoed by a single infrastructure failure.

@@ -378,6 +378,13 @@ jobs:
scenarios:
- jitelthookenabled
- jitelthookenabled_tiered
${{ if in(parameters.testGroup, 'jit-offbydefault-extra') }}:
Copy link
Member

Choose a reason for hiding this comment

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

I like "jit-experimental". I don't think you need "-extra" (We could save that for a version of this that also does JitStress modes, perhaps.)

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM

@AndyAyersMS
Copy link
Member Author

All the jit-experimental runs seem to be testing what I intended. No real reason to wait for them to finish, but I'll do it anyways.

@AndyAyersMS AndyAyersMS merged commit a6f805d into dotnet:master Mar 19, 2020
@AndyAyersMS AndyAyersMS deleted the OffByDefaultJitTestingSupport branch March 19, 2020 00:33
@CarolEidt
Copy link
Contributor

Thanks @AndyAyersMS !

@BruceForstall
Copy link
Member

I noticed that the "runtime-jit-experimental" doesn't appear in the pipelines tree properly under dotnet/runtime: it appears at the top level. @jashook @echesakovMSFT do you know why?

https://dev.azure.com/dnceng/public/_build?view=folders&treeState=XGRvdG5ldCRcZG90bmV0XHJ1bnRpbWU%3D

@echesakov
Copy link
Contributor

@BruceForstall I think it was created this way - you can move it if you want by clicking "Rename/move" and pick a folder

@BruceForstall
Copy link
Member

ok, I moved it. Hopefully that doesn't break anything!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to 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.

6 participants