Remove `AllocLHeap`. #33402

VSadov · 2020-03-09T22:14:05Z

The main difference of AllocLHeap is that it uses per-heap allocation context instead of per-thread one. There is no advantage in that and it results in allocation under-reporting in GetAllocatedBytesForCurrentThread.

It also came up in POH code reviews as unnecessary inconsistency that we do not want to carry forward.

This change unifies to one allocation entry point - Alloc and uses flags to indicate special behaviors.

jkotas · 2020-03-09T23:59:15Z

src/coreclr/src/gc/gcinterface.h

@@ -782,7 +779,7 @@ class IGCHeap {
    virtual Object* AllocAlign8(gc_alloc_context* acontext, size_t size, uint32_t flags) = 0;


Do we really need AllocAlign8 ?

It is substantially different from regular Alloc and needed only on certain platforms. It seems convenient to have implementation in separate methods.

For LOH - we could be forced to use LOH by the caller or decide to use it ourselves based on size. So Alloc has to know about large objects anyways.

The implementation can be inside a separate method in the GC. That's fine.

I am looking at it from the GC interface definition point of view. We do have GC_ALLOC_ALIGN8 flag. It is odd that you can call regular Alloc for all other combination of flags, except when you have this specific flag - you need to call this other method.

I see that GC_ALLOC_ALIGN8 is not used. Perhaps it was meant to be used in Alloc.

Let's unify Alloc and AllocAlign8 at API level.

I see that GC_ALLOC_ALIGN8 is not used. Perhaps it was meant to be used in Alloc.

It has been used in .NET Native / CoreRT that has things more unified, but this unification did not make it into the master GC sources.

Removed AllocAlign8. It did reduce some duplication.

src/coreclr/src/gc/gcinterface.h

… instead of per-thread. There is no advantage in that and results in allocations under-reported in `GetAllocatedBytesForCurrentThread` This change unifies to one allocation entry point - `Alloc` (and its `AllocAlign8` variety)

jkotas · 2020-03-10T20:13:13Z

src/coreclr/src/vm/gchelpers.cpp

+                (GC_ALLOC_ALIGN8_BIAS | GC_ALLOC_ALIGN8) :
+                GC_ALLOC_ALIGN8;
+
+            orObject = (Object *) Alloc(baseSize, flags);


You can merge this with the else part.

jkotas · 2020-03-10T20:23:52Z

src/coreclr/src/vm/gchelpers.cpp

@@ -754,7 +671,7 @@ OBJECTREF AllocateArrayEx(MethodTable *pArrayMT, INT32 *pArgs, DWORD dwNumArgs,
            // cases we care about (single and multi-dim arrays of value types) have an even number of DWORDs
            // in their headers so the alignment requirements for the header and the payload are the same.
            _ASSERTE(((pArrayMT->GetBaseSize() - SIZEOF_OBJHEADER) & 7) == 0);
-            orArray = (ArrayBase *) AllocAlign8(totalSize, flags);
+            orArray = (ArrayBase*)Alloc(totalSize, flags | GC_ALLOC_ALIGN8);


(and other places)

This is about the 3 places where GC_ALLOC_ALIGN8 is used, right?

jkotas · 2020-03-10T21:29:32Z

src/coreclr/src/vm/gchelpers.cpp

        {
+#else   // ! FEATURE_64BIT_ALIGNMENT


Two ifdefs may be more readable than #if / #else / #endif. #ifdefs around unmatched { are hard to read.

It was also causing build issues on arm.

Maoni0 · 2020-03-10T22:00:43Z

src/coreclr/src/gc/gcinterface.h

@@ -7,7 +7,7 @@

 // The major version of the GC/EE interface. Breaking changes to this interface
 // require bumps in the major version number.
-#define GC_INTERFACE_MAJOR_VERSION 4
+#define GC_INTERFACE_MAJOR_VERSION 5


I don't think you need to increase this again, it was already increased with your last checkin to this file. we just need to increase it each time we actually release something

I see. I thought we literally have to update every time we merge breaking changes :-)

Was a bit surprised the number is so low.

every time we make breaking changes that are publicly visible.

Maoni0

…ease. And we already did for this one.

VSadov · 2020-03-11T01:08:53Z

Thanks!!

Dotnet-GitSync-Bot added the area-GC-coreclr label Mar 9, 2020

jkotas reviewed Mar 9, 2020

View reviewed changes

jkotas reviewed Mar 10, 2020

View reviewed changes

src/coreclr/src/gc/gcinterface.h Show resolved Hide resolved

VSadov force-pushed the alloclheap branch from 30cf868 to a8523fe Compare March 10, 2020 02:30

VSadov changed the title ~~[WIP] Remove AllocLHeap.~~ Remove AllocLHeap. Mar 10, 2020

VSadov marked this pull request as ready for review March 10, 2020 04:48

VSadov requested a review from Maoni0 March 10, 2020 04:48

Removed AllocAlign8

09050da

jkotas mentioned this pull request Mar 10, 2020

Consider if there is a better term for non-SOH heaps than UOH #33445

Closed

jkotas reviewed Mar 10, 2020

View reviewed changes

PR feedback - some refactoring to merge duplicate calls to Alloc

cb70485

jkotas reviewed Mar 10, 2020

View reviewed changes

Splited an ifdef/else in two to not cross code blocks.

63ac1a0

Maoni0 reviewed Mar 10, 2020

View reviewed changes

Maoni0 approved these changes Mar 10, 2020

View reviewed changes

No need to update GC_INTERFACE_MAJOR_VERSION more than once per rel…

8af1a53

…ease. And we already did for this one.

VSadov merged commit e4d01e3 into dotnet:master Mar 11, 2020

VSadov deleted the alloclheap branch March 11, 2020 01:09

VSadov mentioned this pull request Mar 11, 2020

Reconcile how LOH allocations are tracked in allocation contexts #13734

Closed

VSadov mentioned this pull request Aug 6, 2020

Test failure: System.Tests.GCTests.GetAllocatedBytesForCurrentThread(size: 1000) #13502

Closed

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `AllocLHeap`. #33402

Remove `AllocLHeap`. #33402

VSadov commented Mar 9, 2020 •

edited

Loading

jkotas Mar 9, 2020

VSadov Mar 10, 2020

VSadov Mar 10, 2020

jkotas Mar 10, 2020 •

edited

Loading

VSadov Mar 10, 2020

jkotas Mar 10, 2020

VSadov Mar 10, 2020

jkotas Mar 10, 2020

jkotas Mar 10, 2020

jkotas Mar 10, 2020

VSadov Mar 10, 2020

jkotas Mar 10, 2020

jkotas Mar 10, 2020

VSadov Mar 10, 2020

Maoni0 Mar 10, 2020

VSadov Mar 10, 2020

VSadov Mar 10, 2020

Maoni0 Mar 10, 2020

Maoni0 left a comment

VSadov commented Mar 11, 2020

		@@ -782,7 +779,7 @@ class IGCHeap {
		virtual Object* AllocAlign8(gc_alloc_context* acontext, size_t size, uint32_t flags) = 0;

Remove AllocLHeap. #33402

Remove AllocLHeap. #33402

Conversation

VSadov commented Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maoni0 left a comment

Choose a reason for hiding this comment

VSadov commented Mar 11, 2020

Remove `AllocLHeap`. #33402

Remove `AllocLHeap`. #33402

VSadov commented Mar 9, 2020 •

edited

Loading

jkotas Mar 10, 2020 •

edited

Loading