[Arm64/Unix] Use portable MP optimized new/alloc #12333

sdmaclea · 2017-06-16T18:32:26Z

No description provided.

sdmaclea · 2017-06-16T18:36:32Z

@arm64-contrib @jkotas @janvorli ~~This is a big win on MP ARM64. Total elapsed test time to run 11K tests reduced 33%.~~

The data point was flawed. The methodology of total elapsed runtime depends on the longest running test. I had disabled the longest running test(s) to fix intermittent failures. Effectively this clipped the tail, and reduce the total elapsed time 33%. It had nothing to do with this change.

sdmaclea · 2017-06-16T18:37:50Z

Code is largely copied from jitinterfacegen.cpp's version of void InitJITHelpers1()

jkotas · 2017-06-16T18:42:34Z

src/vm/arm64/stubs.cpp

+
+    _ASSERTE(g_SystemInfo.dwNumberOfProcessors != 0);
+
+    // TODO-ARM64-NYI !defined(FEATURE_IMPLICIT_TLS) Use helpers optimized w/ inline TLS access...


This TODO is not necessary. FEATURE_IMPLICIT_TLS is as good as what you can get with the handcrafter helpers.

The comment is for the Windows !FEATURE_IMPLICIT_TLS case.

It looks like FEATURE_IMPLICIT_TLS is not defined for Windows. Am I wrong?

In the root clrdefinitions.cmake:

if (CLR_CMAKE_PLATFORM_UNIX OR CLR_CMAKE_TARGET_ARCH_ARM64) add_definitions(-DFEATURE_IMPLICIT_TLS) set(FEATURE_IMPLICIT_TLS 1) endif(CLR_CMAKE_PLATFORM_UNIX OR CLR_CMAKE_TARGET_ARCH_ARM64)

So for ARM64, it is defined for both Windows and Unix.

Done. Closed #12332

sdmaclea · 2017-06-16T22:17:26Z

@dotnet-bot test Ubuntu arm64 Cross Debug Build

sdmaclea · 2017-06-16T22:43:36Z

@jkotas @janvorli This is ready to merge

jkotas · 2017-06-16T22:57:47Z

@jashook Could you please trigger whatever arm64 testing is appropriate?

jashook · 2017-06-16T23:00:00Z

Currently testing is offline and I am looking into the issue. If I cannot find the solution in a reasonable amount of time it is probably worth not blocking.

sdmaclea · 2017-06-16T23:01:18Z

FYI, I did run this through Unix/Arm64 testing locally.

sdmaclea · 2017-06-27T23:34:20Z

@dotnet-bot test Ubuntu arm64 Cross Debug Build

sdmaclea · 2017-06-28T17:19:30Z

ping

jashook · 2017-06-29T19:28:10Z

@dotnet-bot test Windows_NT Arm64 Checked

jashook · 2017-06-29T19:28:16Z

Thank you for waiting @sdmaclea

jashook · 2017-06-30T15:48:13Z

@sdmaclea looks like there is a contract violation

sdmaclea · 2017-06-30T16:43:36Z

@jashook What does a contract violation mean? I have seen these CONTRACT things floating around, but I haven't figured out the syntax or the tool used to check them.

jkotas · 2017-06-30T16:50:18Z

The CONTRACT things are described in https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-code-guide.md#2.10

They get checked as asserts at runtime - on Windows only. The contract violation is basically a special form of assertion failure.

I have fixed a contract violation in #12542. Was this change included in the run?

jashook · 2017-06-30T16:53:08Z

It is hard to say, I can restart the job to to see if it repros now.

@dotnet-bot test Windows_NT Arm64 Checked

sdmaclea · 2017-07-03T17:33:35Z

Build timeout
@dotnet-bot test Windows_NT Arm64 Checked

sdmaclea · 2017-07-05T15:12:22Z

@jkotas Contract violation occurs with #12542. My guess is the CV is in the arm64 tip. Triggered "Windows_NT arm64 Cross Checked Build and Test" on #12436 to test hypothesis.

jashook · 2017-07-05T20:03:40Z

@sdmaclea Windows arm64 jobs should be clean currently.

sdmaclea · 2017-07-05T20:23:07Z

@jashook @jkotas Confirming #12436 completed w/o contract violation.

sdmaclea · 2017-07-05T20:23:34Z

@dotnet-bot test Ubuntu arm64 Cross Debug Build

sdmaclea · 2017-07-07T15:57:37Z

@jashook FWIW Looks like arm64_debug_small_page_size_prtest pass, but arm64_debug_small_page_size_flow_prtest do not.

sdmaclea · 2017-07-26T14:49:26Z

@jkotas I removed the contract line I added. Since I do not have a Windows Arm64 platform, this the CV is not something I can easily debug. I'll open an issue to add the contract.

sdmaclea · 2017-08-01T15:50:02Z

@jkotas @dotnet/arm64-contrib please review and/or merge

jkotas · 2017-08-01T21:08:23Z

@dotnet/arm64-contrib Are Windows arm64 tests passing on this change?

I see that Windows_NT arm64 Cross Debug Build is green, but I do not see any tests run in the log.

Remove change to InitJITHelpers1() contract

IIRC, contracts that failed on this earlier were not related to InitJITHelpers1. I do not think that there was anything wrong with the InitJITHelpers1 contract that you have added. It is ok to look into it independently since you have filled issue on it.

sdmaclea · 2017-08-01T21:23:05Z

@dotnet-bot test Windows_NT arm64 Checked

sdmaclea · 2017-08-16T19:30:21Z

@jkotas Since I do not have Windows test platform to debug Contract violation, I disabled the change for Windows and changed #13053 to match.

jkotas

There is some chance that the contract violation on Windows flagged a problem that exists on non-Windows as well (the runtime contract checks are Windows-only). I assume that you have done a test pass to verify that there are not regressions on Linux. (The regression will likely show up as a intermittent crash during GC.)

sdmaclea · 2017-08-16T20:02:15Z

@jkotas

I assume that you have done a test pass to verify that there are not regressions on Linux.

Yes. I have been using the patch for all my runs for the last two months. Either way, I will try to get someone from the QDT windows team to debug the Contract violation.

The regression will likely show up as a intermittent crash during GC.

Will GC stress testing help flush it out?

arm64 GC is not perfect yet. there are still interrmittent failures. So It may not be obvious that it is a CV related issue.

jkotas · 2017-08-16T20:41:29Z

I have been using the patch for all my runs for the last two months

This should be good enough.

Will GC stress testing help flush it out?

Maybe. It is best to debug the known problem on Windows first, I think.

sdmaclea · 2017-08-17T17:12:10Z

@dotnet-bot test Ubuntu arm64 Cross Debug Build

sdmaclea · 2017-08-22T18:56:16Z

Please merge

sdmaclea · 2017-08-23T16:01:45Z

src/vm/arm64/stubs.cpp

@@ -1086,6 +1087,31 @@ void JIT_TailCall()
 #if !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE)
 void InitJITHelpers1()
 {
+#ifdef FEATURE_PAL // TODO


This should be removed to support Windows

dnfclas added the cla-already-signed label Jun 16, 2017

jkotas reviewed Jun 16, 2017

View reviewed changes

sdmaclea force-pushed the PR-ARM64-MP-NEW branch from 773d275 to 76e8ac2 Compare July 26, 2017 14:45

[Arm64/Unix] Use portable MP optimized new/alloc

527d921

sdmaclea force-pushed the PR-ARM64-MP-NEW branch from 76e8ac2 to 527d921 Compare August 16, 2017 19:24

sdmaclea changed the title ~~[Arm64] Use portable MP optimized new/alloc~~ [Arm64/Unix] Use portable MP optimized new/alloc Aug 16, 2017

jkotas approved these changes Aug 16, 2017

View reviewed changes

jkotas merged commit f0e8660 into dotnet:master Aug 22, 2017

sdmaclea commented Aug 23, 2017

View reviewed changes

karelz modified the milestone: 2.1.0 Aug 28, 2017

sdmaclea deleted the PR-ARM64-MP-NEW branch December 4, 2017 16:30

sdmaclea mentioned this pull request Jan 31, 2020

[Arm64 Windows] Use portable MP optimized new/alloc dotnet/runtime#8641

Closed


		_ASSERTE(g_SystemInfo.dwNumberOfProcessors != 0);

		// TODO-ARM64-NYI !defined(FEATURE_IMPLICIT_TLS) Use helpers optimized w/ inline TLS access...

[Arm64/Unix] Use portable MP optimized new/alloc #12333

[Arm64/Unix] Use portable MP optimized new/alloc #12333

Uh oh!

Conversation

sdmaclea commented Jun 16, 2017

Uh oh!

sdmaclea commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdmaclea commented Jun 16, 2017

Uh oh!

jkotas Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

sdmaclea Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

janvorli Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

sdmaclea Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

sdmaclea commented Jun 16, 2017

Uh oh!

sdmaclea commented Jun 16, 2017

Uh oh!

jkotas commented Jun 16, 2017

Uh oh!

jashook commented Jun 16, 2017

Uh oh!

sdmaclea commented Jun 16, 2017

Uh oh!

sdmaclea commented Jun 27, 2017

Uh oh!

sdmaclea commented Jun 28, 2017

Uh oh!

jashook commented Jun 29, 2017

Uh oh!

jashook commented Jun 29, 2017

Uh oh!

jashook commented Jun 30, 2017

Uh oh!

sdmaclea commented Jun 30, 2017

Uh oh!

jkotas commented Jun 30, 2017

Uh oh!

jashook commented Jun 30, 2017

Uh oh!

sdmaclea commented Jul 3, 2017

Uh oh!

sdmaclea commented Jul 5, 2017

Uh oh!

jashook commented Jul 5, 2017

Uh oh!

sdmaclea commented Jul 5, 2017

Uh oh!

sdmaclea commented Jul 5, 2017

Uh oh!

sdmaclea commented Jul 7, 2017

Uh oh!

sdmaclea commented Jul 26, 2017

Uh oh!

sdmaclea commented Aug 1, 2017

Uh oh!

jkotas commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdmaclea commented Aug 1, 2017

Uh oh!

sdmaclea commented Aug 16, 2017

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

sdmaclea commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

sdmaclea commented Jun 16, 2017 •

edited

Loading

jkotas Jun 16, 2017 •

edited

Loading

jkotas commented Aug 1, 2017 •

edited

Loading

sdmaclea commented Aug 16, 2017 •

edited

Loading