Remove hosting methods that always return false #9930

Merged
merged 9 commits into from Mar 6, 2017

Conversation

Projects
None yet
4 participants
@AustinWise
Contributor

AustinWise commented Mar 3, 2017

When commit 1476776, as part of #9603, removed all the code under FEATURE_INCLUDE_ALL_INTERFACES, it left behind several functions in src/vm/util.hpp that just return false. This commit is a follow up to #9899 that removes the remaining functions.

One of the commits in this PR changes gc.cpp. While the changes are part of an IFDEF that is not active in corert, I'll propagate the changes to corert unless instructed to do otherwise.

src/gc/gc.cpp
@@ -5339,18 +5339,13 @@ void gc_heap::gc_thread_function ()
bool virtual_alloc_commit_for_heap(void* addr, size_t size, int h_number)
{
#if defined(MULTIPLE_HEAPS) && !defined(FEATURE_REDHAWK) && !defined(FEATURE_PAL)
- // Currently there is no way for us to specific the numa node to allocate on via hosting interfaces to
- // a host. This will need to be added later.
- if (!CLRMemoryHosted())

This comment has been minimized.

@jkotas

jkotas Mar 4, 2017

Member

Could you please keep the CLRMemoryHosted() check here and put #ifndef FEATURE_CORECLR around it instead?

cc @swgillespie

@jkotas

jkotas Mar 4, 2017

Member

Could you please keep the CLRMemoryHosted() check here and put #ifndef FEATURE_CORECLR around it instead?

cc @swgillespie

src/vm/util.hpp
- return hr; \
- } \
-}
+#define ComCallHostNotificationHR() ReverseEnterRuntimeHolderNoThrow REHolder;

This comment has been minimized.

@jkotas

jkotas Mar 4, 2017

Member

ReverseEnterRuntimeHolder does nothing without the hosting. You can delete it from everywhere.

@jkotas

jkotas Mar 4, 2017

Member

ReverseEnterRuntimeHolder does nothing without the hosting. You can delete it from everywhere.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Mar 4, 2017

Member

Looks good modulo comments. Thanks for cleaning it up!

Member

jkotas commented Mar 4, 2017

Looks good modulo comments. Thanks for cleaning it up!

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Mar 5, 2017

Contributor

I did an interactive rebase onto master, editing the CLRMemoryHosted commit use an IFDEF in gc.cpp rather than removing the call to CLRMemoryHosted. I also added a commit that removed the EnterRuntime and LeaveRuntime calls and their associated holders.

Contributor

AustinWise commented Mar 5, 2017

I did an interactive rebase onto master, editing the CLRMemoryHosted commit use an IFDEF in gc.cpp rather than removing the call to CLRMemoryHosted. I also added a commit that removed the EnterRuntime and LeaveRuntime calls and their associated holders.

src/gc/gc.cpp
@@ -5338,7 +5338,7 @@ void gc_heap::gc_thread_function ()
bool virtual_alloc_commit_for_heap(void* addr, size_t size, int h_number)
{
-#if defined(MULTIPLE_HEAPS) && !defined(FEATURE_REDHAWK) && !defined(FEATURE_PAL)
+#if defined(MULTIPLE_HEAPS) && !defined(FEATURE_REDHAWK) && !defined(FEATURE_PAL) && !defined(FEATURE_CORECLR)

This comment has been minimized.

@jkotas

jkotas Mar 5, 2017

Member

I am sorry I was not clear ... this should be something like:

#if defined(MULTIPLE_HEAPS) && !defined(FEATURE_REDHAWK) && !defined(FEATURE_PAL)

#if !defined(FEATURE_CORECLR)
     // Currently there is no way for us to specific the numa node to allocate on via hosting interfaces to  
     // a host. This will need to be added later. 
     if (!CLRMemoryHosted())
#endif
     {
...

The numa support should stay enabled to CoreCLR - it is enabled for CoreCLR in other places.

@jkotas

jkotas Mar 5, 2017

Member

I am sorry I was not clear ... this should be something like:

#if defined(MULTIPLE_HEAPS) && !defined(FEATURE_REDHAWK) && !defined(FEATURE_PAL)

#if !defined(FEATURE_CORECLR)
     // Currently there is no way for us to specific the numa node to allocate on via hosting interfaces to  
     // a host. This will need to be added later. 
     if (!CLRMemoryHosted())
#endif
     {
...

The numa support should stay enabled to CoreCLR - it is enabled for CoreCLR in other places.

This comment has been minimized.

@AustinWise

AustinWise Mar 5, 2017

Contributor

Oh, whoops, I misunderstood. I pushed a new commit with a fix for this.

@AustinWise

AustinWise Mar 5, 2017

Contributor

Oh, whoops, I misunderstood. I pushed a new commit with a fix for this.

HandleHolder token;
BOOL bReverted = FALSE;
- bOK = RevertIfImpersonated(&bReverted, &token, &affinityHolder);

This comment has been minimized.

@jkotas

jkotas Mar 5, 2017

Member

Should this have just the affinityHolder argument removed, but the call should stay?

@jkotas

jkotas Mar 5, 2017

Member

Should this have just the affinityHolder argument removed, but the call should stay?

This comment has been minimized.

@AustinWise

AustinWise Mar 5, 2017

Contributor

Whoops, added it back.

@AustinWise

AustinWise Mar 5, 2017

Contributor

Whoops, added it back.

Add back calls to RevertIfImpersonated and GCX_PREEMP.
I accidentally deleted the call to RevertIfImpersonated instead of just
removing an extra parameter.

When I removed the HR_LEAVE_RUNTIME_HOLDER macro from windowsruntime.h,
I not only removed a LeaveRuntimeHolder but also a GCX_PREEMP. So I added
it back. The holder and GCX_PREEMP where only inserted when the
FEATURE_LEAVE_RUNTIME_HOLDER macro was defined. Since it is always defined,
I removed it. Also as I understand it, you would always want to have a
GCX_PREEMP before calling into the Windows API as not to block the GC,
so I'm not sure why you would want to disable it.
@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Mar 5, 2017

Contributor

I reviewed my commit that removed EnterRuntime and LeaveRuntime and found I had removed some calls to GCX_PREEMP() in windowsruntime.h. I added them back in along with the call to RevertIfImpersonated.

Contributor

AustinWise commented Mar 5, 2017

I reviewed my commit that removed EnterRuntime and LeaveRuntime and found I had removed some calls to GCX_PREEMP() in windowsruntime.h. I added them back in along with the call to RevertIfImpersonated.

@jkotas jkotas merged commit 51e968b into dotnet:master Mar 6, 2017

13 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details
@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Mar 6, 2017

Member

Thank you for cleaning this up!

Member

jkotas commented Mar 6, 2017

Thank you for cleaning this up!

@AustinWise AustinWise deleted the AustinWise:removeHosting branch Mar 6, 2017

YongseopKim added a commit to YongseopKim/coreclr that referenced this pull request Mar 10, 2017

Remove hosting methods that always return false (#9930)
* Remove CLRIoCompletionHosted as it always returns false.
* Remove CLRSyncHosted as it always returns false.
* Remove CLRMemoryHosted as it always returns false.
* Remove CLRTaskHosted as it always returns false.
* Remove CLRAssemblyHosted, CLRGCHosted,and CLRSecurityHosted.
They are not called.
* Remove IsThreadPoolHosted as it always returns false.
* Remove EnterRuntime and LeaveRuntime, as they do nothing.
* Add back calls to RevertIfImpersonated and GCX_PREEMP.

I accidentally deleted the call to RevertIfImpersonated instead of just
removing an extra parameter.

When I removed the HR_LEAVE_RUNTIME_HOLDER macro from windowsruntime.h,
I not only removed a LeaveRuntimeHolder but also a GCX_PREEMP. So I added
it back. The holder and GCX_PREEMP where only inserted when the
FEATURE_LEAVE_RUNTIME_HOLDER macro was defined. Since it is always defined,
I removed it. Also as I understand it, you would always want to have a
GCX_PREEMP before calling into the Windows API as not to block the GC,
so I'm not sure why you would want to disable it.

jorive added a commit to guhuro/coreclr that referenced this pull request May 4, 2017

Remove hosting methods that always return false (#9930)
* Remove CLRIoCompletionHosted as it always returns false.
* Remove CLRSyncHosted as it always returns false.
* Remove CLRMemoryHosted as it always returns false.
* Remove CLRTaskHosted as it always returns false.
* Remove CLRAssemblyHosted, CLRGCHosted,and CLRSecurityHosted.
They are not called.
* Remove IsThreadPoolHosted as it always returns false.
* Remove EnterRuntime and LeaveRuntime, as they do nothing.
* Add back calls to RevertIfImpersonated and GCX_PREEMP.

I accidentally deleted the call to RevertIfImpersonated instead of just
removing an extra parameter.

When I removed the HR_LEAVE_RUNTIME_HOLDER macro from windowsruntime.h,
I not only removed a LeaveRuntimeHolder but also a GCX_PREEMP. So I added
it back. The holder and GCX_PREEMP where only inserted when the
FEATURE_LEAVE_RUNTIME_HOLDER macro was defined. Since it is always defined,
I removed it. Also as I understand it, you would always want to have a
GCX_PREEMP before calling into the Windows API as not to block the GC,
so I'm not sure why you would want to disable it.

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment