WIP: Simplify lowest level compiler allocation classes #3396

mstoodle · 2018-12-23T18:36:27Z

This PR prereqs a change in OpenJ9 to avoid breaking OpenJ9, so
it has been marked WIP. Reviews are welcome at this stage.

The lowest level memory allocation classes in the OMR compiler
have a complicated hierarchy and relationship that many people
have struggled to understand and maintain. The complexity came
about trying to hold a unified picture of memory allocation
between OMR (which just uses a raw malloc allocator) and OpenJ9
(which uses J9MemorySegments allocated from the VM a.k.a.
"the system"). Unfortunately, this unity did not materialize
and so these lower level allocation classes are not actually
shared with OpenJ9.

The set of changes in this commit is motivated by the effort
to try to compile the basic JitBuilder files in compiler/ilgen
as part of OpenJ9. In this commit, I brought over the code from
OpenJ9's SystemSegmentProvider and refactored it so that it
could operate in OMR using TR::MemorySegments rather than
J9MemorySegments. I changed its name to SegmentAllocator and
declare the SegmentAllocator as the base layer for the higher
level allocation support for e.g. regions and STL and other
objects in the compiler component.

This commit is the first stage in a series of coordinated
changes to OMR and OpenJ9 to migrate from the "old" memory
allocation scheme to the "new" memor allocation scheme. In
this change, most of the new code is implemented inside
a new compiler/env/mem directory which contains any files
from compiler/env that required substantial modification.
This commit also introduces a temporary macro called,
uninspiringly, OLD_MEMORY: if OLD_MEMORY is defined then
the old OMR memory allocation classes will be built rather
than the new ones. By default after this commit, OMR will
build with the new memory allocation classes (e.g. into
jitbuilder and compilertest). Most consumers of the OMR
compiler should experience zero functional differences
as the memory classes being changed are at the lowest levels
of the compiler implementation (well lower than the facilities
most compiler developers use directly).

The SegmentAllocator will hold a BackingMemoryAllocator and a
RawAllocator. In fact, the BackingMemoryAllocator in OMR is
an only slightly beefed up RawAllocator which returns memory
wrapped in a TR::MemorySegment. I deleted the SegmentPool code
because nobody was using it. The DebugSegmentProvider code has
been moved into DebugRawAllocator (subclass of TR::RawAllocator)
as well as DebugSegmentAllocator (subclass of TR::SegmentAllocator).

I added some tracing output to the SegmentAllocator and
BackingMemoryAllocator to track segments as they are allocated
and deallocated (see MEMLOG macros in each file). This debug
output uses printfs due to the primordial environment of the
memory allocators, and is activated by environment variables
OMRDebug_BackingMemory=1 or 2 or 3 (increasing detail)
OMRDebug_SegmetnAllocator=1 or 2 or 3 (increasing detail)

While debugging the memory allocation layers, I discovered that
the original list management for segments was not very efficient
in that new segments were typically added at the end of lists but
searched starting from the beginning. This commit changes the
order of search to require fewer probes (less than half the
probes required in some very unscientific studies). I doubt
it will have any performance impact in most workloads but
it felt better to fix it.

BackingMemoryAllocator, SegmentAllocator, and DebugSegmentAllocator
are all extensible classes to facilitate a similar refactoring
extravaganza to be inflicted upon OpenJ9 very soon.

mstoodle · 2018-12-23T23:18:15Z

genie-omr build all

mstoodle · 2018-12-23T23:26:24Z

genie-omr build all

mstoodle · 2018-12-24T01:41:36Z

genie-omr build all

mstoodle · 2018-12-24T02:07:40Z

genie-omr build all

mstoodle · 2018-12-24T04:12:08Z

genie-omr build arm

mstoodle · 2018-12-24T04:23:49Z

genie-omr build all

mstoodle · 2018-12-24T04:44:23Z

Note to reviewers; I still have to update the compiler memory documentation in this PR.

dsouzai · 2018-12-24T14:51:31Z

compiler/control/CompileMethod.cpp

+      debugSegmentAllocator : defaultSegmentAllocator;
+#endif
+
+   TR::Region dispatchRegion(segmentAllocator, rawAllocator);


segmentAllocator isn't defined when using OLD_MEMORY; I guess the variable is named segmentProvider.

Given the change below (ie using segmentProvider for OLD_MEMORY, and segmentAllocator for the new stuff), it seems like you're going to have to duplicate the initialization of the TR::Region in each of the ifdef sections

good catch, will fix

dsouzai · 2018-12-24T15:06:28Z

compiler/env/mem/OMRBackingMemoryAllocator.cpp

+   void *p = _rawAllocator.allocate(sizeToAllocate, hint);
+   TR::MemorySegment *newSegment = new (_rawAllocator) TR::MemorySegment(p, sizeToAllocate);
+   if (newSegment == NULL)
+      throw std::bad_alloc();


Unless std::nothrow_t tag is specified as a parameter to the placement new (which in this case it isn't), OMR::RawAllocator throws a std::bad_alloc(), so this is redundant.

another good catch, will remove the check

dsouzai · 2018-12-24T15:09:25Z

compiler/env/mem/OMRBackingMemoryAllocator.cpp

+         break;
+         }
+      }
+   TR_ASSERT(found, "Request to deallocate segment not found in allocated segment list");


How "fatal" is it if we can't find the segment in this deque?

At best it's going to be a memory leak, and at worst I suppose it's a sign that memory corruption is likely happening in the compilation. I suppose a TR_ASSERT_FATAL would be a better option.

dsouzai · 2018-12-24T15:11:50Z

compiler/env/mem/OMRBackingMemoryAllocator.cpp

+   while (!_allocatedSegments.empty())
+      {
+      TR::MemorySegment &segment = _allocatedSegments.front().get();
+      deallocate(segment);


Just curious as to whether BackingMemoryAllocator can be extensible, and if so, whether we want to enforce that the OMR version of this is guaranteed to get called.

I'm not sure that's appropriate: if someone creates an extension of BackingMemoryAllocator that implements a different allocate() function then presumably they should have the ability to also provide a different deallocate() function that matches?

Someone who did that would probably want to further break down deallocate() so they could reuse some parts of it without necessarily using other parts (exercise left for those who need it :) ?)

Yeah fair enough; I figured I'd mention it jic :)

Looking at this a second time, wouldn't this result in the code going through the _allocatedSegments list for every iteration of the loop (since deallocate goes through the list to see if the segment is in the list)?

no, because they start at the same end of the list :). It asks what the head is, then removes the head (so deallocate just picks off the first segment). It might be possible to refactor deallocate to not rely on it and it probably deserves a comment, but i guess I went the easy way.

For the record, I found the list iteration in the earlier implementation, which I initially duplicated, was not as efficient as it should be, so I tweaked it a bit to make it better. So good catch :) .

Heh I see, sneaky :P

dsouzai · 2018-12-24T15:15:01Z

compiler/env/mem/OMRBackingMemoryAllocator.cpp

+
+
+TR::MemorySegment &
+OMR::BackingMemoryAllocator::allocate(size_t requiredSize, void * hint)


This method doesn't check the _allocationLimit even though that concept exists at the BackingMemoryAllocator level.

Um, yeah...good point :)

dsouzai · 2018-12-24T15:18:00Z

What is the reason OMRCompilerEnv is in the mem dir?

dsouzai · 2018-12-24T16:27:14Z

compiler/ilgen/OMRTypeDictionary.hpp

@@ -203,7 +208,8 @@ class TypeDictionary
      MemoryManager();
      ~MemoryManager();

-      TR::SegmentProvider *_segmentProvider;
+      TR::BackingMemoryAllocator *_backingMemoryAllocator;


Probably worth having a comment here stating how this should never be used by anything but _segmentAllocator.

dsouzai · 2018-12-24T16:29:07Z

jitbuilder/control/Jit.cpp

@@ -120,12 +123,14 @@ initializeJitBuilder(TR_RuntimeHelper *helperIDs, void **helperAddresses, int32_
   // Create a bootstrap raw allocator.
   //
   TR::RawAllocator rawAllocator;
+   TR::BackingMemoryAllocator backingAllocator(rawAllocator, 1<< 20);


Same comment as above. Although now that I think of it (and I'm not saying it should be done here), but since the backingAllocator and segmentAllocator seem to always go together at least during initialization, perhaps it's worth having them encapsulated in a struct so that the lifetimes of both are linked in places where it makes sense that they be linked?

They don't have to be linked together. That's just how they're used currently....I don't believe there's a requirement for the BackingMemoryAllocator to die just because the SegmentAllocator dies...

I don't believe there's a requirement for the BackingMemoryAllocator to die just because the SegmentAllocator dies...

Yeah that's a good point; I guess I was just looking at the current uses of it, and it seems to have the same lifetime as the SegmentAllocator. That said, there's no reason that in the future you couldn't initialize another SegmentAllocator once the first one has been destroyed. Still, it's worth maybe putting some warning comments to alert people to the fact that this is a very subtle piece of code.

doc/compiler/memory/MemoryManager.md

dsouzai

Documentation lgtm (aside from the comments already made)

dsouzai · 2019-01-07T19:42:34Z

Btw, forgot to mention earlier, but the copyrights need to be updated :)

mstoodle · 2019-01-07T21:02:28Z

Ah, now I understand the source of your question: the mem directory is temporary (yes, poorly named but hopefully not long lived): it holds the files requiring substantial changes between "old" and "new" memory management.

dsouzai

lgtm

doc/compiler/memory/MemoryManager.md

Leonardo2718 · 2019-01-08T16:09:43Z

doc/compiler/memory/MemoryManager.md

+RawAllocator classes:
+----------------------------+         +-----------------------------+
+|                            |  using  |                             |
+|     TR::RawAllocator       +<--------+     OMR::RawAllocator       |


I find the word using a bit confusing here. Is it referring to the fact that OMR::RawAllocator should be "used" via TR::RawAllocator? Why not just show the inheritance relationship instead?

I didn't see a good way to represent it, so "confusing" is a perfectly apt word to use.

OMR::RawAllocator is placed into the TR namespace via a using statement:
namespace TR { using OMR::RawAllocator };

doc/compiler/memory/MemoryManager.md

mstoodle · 2019-01-08T22:13:34Z

genie-omr build all

mstoodle · 2019-01-08T22:31:42Z

genie-omr build all

mstoodle · 2019-01-08T22:44:37Z

genie-omr build all

The lowest level memory allocation classes in the OMR compiler have a complicated hierarchy and relationship that many people have struggled to understand and maintain. The complexity came about trying to hold a unified picture of memory allocation between OMR (which just uses a raw malloc allocator) and OpenJ9 (which uses J9MemorySegments allocated from the VM a.k.a. "the system"). Unfortunately, this unity did not materialize and so these lower level allocation classes are not actually shared with OpenJ9. The set of changes in this commit is motivated by the effort to try to compile the basic JitBuilder files in compiler/ilgen as part of OpenJ9. In this commit, I brought over the code from OpenJ9's SystemSegmentProvider and refactored it so that it could operate in OMR using TR::MemorySegments rather than J9MemorySegments. I changed its name to SegmentAllocator and declare the SegmentAllocator as the base layer for the higher level allocation support for e.g. regions and STL and other objects in the compiler component. This commit is the first stage in a series of coordinated changes to OMR and OpenJ9 to migrate from the "old" memory allocation scheme to the "new" memor allocation scheme. In this change, *most* of the new code is implemented inside a new compiler/env/mem directory which contains any files from compiler/env that required substantial modification. This commit also introduces a temporary macro called, uninspiringly, OLD_MEMORY: if OLD_MEMORY is defined then the old OMR memory allocation classes will be built rather than the new ones. By default after this commit, OMR will build with the *new* memory allocation classes (e.g. into jitbuilder and compilertest). Most consumers of the OMR compiler should experience *zero* functional differences as the memory classes being changed are at the lowest levels of the compiler implementation (well below than the facilities most compiler developers use directly). The SegmentAllocator will hold a BackingMemoryAllocator which will itself hold a RawAllocator. In fact, the BackingMemoryAllocator in OMR is an only slightly beefed up RawAllocator which returns memory wrapped in a TR::MemorySegment. The SegmentPool code was removed because nobody was using it. he DebugSegmentProvider code has been moved into DebugRawAllocator (subclass of TR::RawAllocator) as well as DebugSegmentAllocator (subclass of TR::SegmentAllocator). I added some tracing output to the SegmentAllocator and BackingMemoryAllocator to track segments as they are allocated and deallocated (see MEMLOG macros in each file). This debug output uses printfs due to the primordial environment of the memory allocators, and is activated by environment variables OMRDebug_BackingMemory=1 or 2 or 3 (increasing detail) OMRDebug_SegmentAllocator=1 or 2 or 3 (increasing detail) While debugging the memory allocation layers, I discovered that the original list management for segments was less efficient than it could be, in that new segments were typically added at the end of lists but searched starting from the beginning. This commit changes the order of search to require fewer probes (less than half the probes required in some very unscientific studies with very simple compilations). I doubt it will have any performance impact in most workloads but it felt better to fix it. BackingMemoryAllocator, SegmentAllocator, and DebugSegmentAllocator are all extensible classes to facilitate a similar refactoring extravaganza to be inflicted upon OpenJ9 very soon. Signed-off-by: Mark Stoodley <[email protected]>

The compiler's lowest level memory allocation implementation is changing so provide documention for the new scheme. But since the old implementation is still available via the OLD_MEMORY macro, the older documentation is not removed, but moved to a file prefixed by "Old" and with comments at the top directing towards the new file. Once all downstream projects have stopped using the old scheme, this older documentation will be removed. Signed-off-by: Mark Stoodley <[email protected]>

Signed-off-by: Mark Stoodley <[email protected]>

mstoodle · 2019-01-09T14:40:59Z

genie-omr build all

Leonardo2718 · 2019-01-09T15:12:36Z

doc/compiler/memory/MemoryManager.md

+fragmentation from the raw allocator. For example, the default
+method compilation requests will construct a backing memory allocator
+with a default segment size of 64KB (`1 << 16`), which means all 
+allocations from the backing memory allocator will rounded up to


Suggested change

allocations from the backing memory allocator will rounded up to

allocations from the backing memory allocator will be rounded up to

Leonardo2718 · 2019-01-09T15:16:23Z

doc/compiler/memory/MemoryManager.md

+The backing memory allocator tracks how much memory has been allocated
+via `_bytesAllocated` which can be queried via `bytesAllocated()`. The
+backing memory allocator can also be directed to enforce an
+allocation limit via a constructor option which sets the `_allocationLimit`


When you say "constructor option", do you mean "constructor parameter"?

Suggested change

allocation limit via a constructor option which sets the `_allocationLimit`

allocation limit via a constructor parameter which sets the `_allocationLimit`

Leonardo2718 · 2019-01-09T15:27:14Z

doc/compiler/memory/MemoryManager.md

+required by the compiler).
+
+This reuse has one restriction to simplify the reuse algorithm:
+only segments smaller than the default segment size (which will


It's not completely clear to me what the "default segment size" is referring to. I assume it's the "minimum" size of segments returned by the segment allocator; the 64KB carved out of some other, larger segment in the example above. Is that correct?

yes, that's correct; i'll see if I can make that more obvious

Leonardo2718 · 2019-01-09T15:32:09Z

doc/compiler/memory/MemoryManager.md

+free segments, which are `TR::MemorySegment`s that have already
+been cut up into the default allocation size. Because all
+allocation requests are rounded up to this size anyway (except
+for larger memory requests), any segment on this free list can


I find the phrasing of this sentence slightly confusing because, if I understand correctly, larger memory requests are rounded up to a multiple of the base size. Can the sentence be changed to something like:

Because all allocation requests smaller than the base are rounded up, ...

?

i'll try to reword that phrase to make it less awkward

Leonardo2718 · 2019-01-09T15:36:27Z

doc/compiler/memory/MemoryManager.md

+`TR::MemorySegment` allocated by the backing memory allocator
+that has not been fully consumed yet. If the request can
+be satisfied from this memory segment, then a new
+`TR::MemorySegment` will be created (using the raw allocator)


Might also be worth mentioning where the raw allocator is coming from. I assume it's the one inside the backing allocator but I'd prefer to have that explicitly stated for clarity.

that makes sense.

mstoodle · 2019-01-10T14:15:44Z

There are a couple more changes coming already, inspired by the work in OpenJ9. I would characterize these changes as mostly "cosmetic" in that they do not impact how the memory allocation classes operate; the changers make them more useful for reuse in other projects:

I have now realized the awkwardness in the using construct for mapping an extensible class into the TR namespace, so I'll be promoting many of these classes to full OMR_EXTENSIBLE classes and also bring in self()
some of the fields in OMR classes are marked as private but are useful to reference directly in subclasses, so I'll be promoting those fields to protected

...probably more to come...

mstoodle · 2019-10-22T17:48:28Z

This pull request no longer reflects the direction I think this work should take. Following the compiler architecture meeting where we discussed this design, I received significant input from others that motivated a somewhat different take. I also gained an improved understanding of how RawAllocator interacts with STL allocators that makes some of this design, as written, problematic. I'm going to close this PR at this point, since it's obviously stale.

Note to any (optimistically presumed) subsequent readers: please feel free to contact me to discuss this work if you're interested :) .

mstoodle requested review from 0xdaryl, charliegracie and Leonardo2718 as code owners December 23, 2018 18:36

mstoodle added the comp:compiler label Dec 23, 2018

mstoodle mentioned this pull request Dec 23, 2018

WIP: Tolerate upcoming OMR compiler memory allocator changes eclipse-openj9/openj9#4148

Open

mstoodle force-pushed the simplifyMemory branch from 3c5659b to 2174da2 Compare December 23, 2018 18:50

mstoodle force-pushed the simplifyMemory branch from 2174da2 to 60b0563 Compare December 23, 2018 23:25

mstoodle force-pushed the simplifyMemory branch 5 times, most recently from 2768da9 to 224c7ae Compare December 24, 2018 01:15

mstoodle force-pushed the simplifyMemory branch from 224c7ae to bc092a3 Compare December 24, 2018 02:07

mstoodle force-pushed the simplifyMemory branch from bc092a3 to b8e4fb5 Compare December 24, 2018 04:11

mstoodle mentioned this pull request Dec 24, 2018

WIP: Simplify compiler basic memory allocation classes #3298

Closed

dsouzai reviewed Dec 24, 2018

View reviewed changes

dsouzai reviewed Jan 7, 2019

View reviewed changes

doc/compiler/memory/MemoryManager.md Outdated Show resolved Hide resolved

dsouzai reviewed Jan 7, 2019

View reviewed changes

doc/compiler/memory/MemoryManager.md Outdated Show resolved Hide resolved

dsouzai reviewed Jan 7, 2019

View reviewed changes

doc/compiler/memory/MemoryManager.md Outdated Show resolved Hide resolved

dsouzai approved these changes Jan 7, 2019

View reviewed changes

mstoodle force-pushed the simplifyMemory branch 2 times, most recently from c1394f5 to 69880ab Compare January 7, 2019 22:10

dsouzai approved these changes Jan 7, 2019

View reviewed changes

Leonardo2718 reviewed Jan 8, 2019

View reviewed changes

mstoodle force-pushed the simplifyMemory branch from 69880ab to 292ef4e Compare January 8, 2019 22:13

mstoodle force-pushed the simplifyMemory branch from 292ef4e to db1c208 Compare January 8, 2019 22:25

mstoodle force-pushed the simplifyMemory branch from db1c208 to 1d4edf3 Compare January 8, 2019 22:44

mstoodle added 3 commits January 9, 2019 09:38

remove #include OMROptions.hpp from OMROptions.hpp

d1ce3e6

Signed-off-by: Mark Stoodley <[email protected]>

mstoodle force-pushed the simplifyMemory branch from 1d4edf3 to d1ce3e6 Compare January 9, 2019 14:39

Leonardo2718 reviewed Jan 9, 2019

View reviewed changes

0xdaryl mentioned this pull request Jan 16, 2019

Agenda for Jan. 17, 2019 Compiler Architecture Meeting #3466

Closed

mstoodle closed this Oct 22, 2019



		TR::MemorySegment &
		OMR::BackingMemoryAllocator::allocate(size_t requiredSize, void * hint)

	allocations from the backing memory allocator will rounded up to
	allocations from the backing memory allocator will be rounded up to

	allocation limit via a constructor option which sets the `_allocationLimit`
	allocation limit via a constructor parameter which sets the `_allocationLimit`

WIP: Simplify lowest level compiler allocation classes #3396

WIP: Simplify lowest level compiler allocation classes #3396

Conversation

mstoodle commented Dec 23, 2018

mstoodle commented Dec 23, 2018

mstoodle commented Dec 23, 2018

mstoodle commented Dec 24, 2018

mstoodle commented Dec 24, 2018

mstoodle commented Dec 24, 2018

mstoodle commented Dec 24, 2018

mstoodle commented Dec 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsouzai Dec 24, 2018 • 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

dsouzai Jan 7, 2019 • 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

dsouzai commented Dec 24, 2018 • 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

dsouzai left a comment

Choose a reason for hiding this comment

dsouzai commented Jan 7, 2019

mstoodle commented Jan 7, 2019

dsouzai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoodle commented Jan 8, 2019

mstoodle commented Jan 8, 2019

mstoodle commented Jan 8, 2019

mstoodle commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoodle Jan 10, 2019 • 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

mstoodle commented Jan 10, 2019 • edited Loading

mstoodle commented Oct 22, 2019

dsouzai Dec 24, 2018 •

edited

Loading

dsouzai Jan 7, 2019 •

edited

Loading

dsouzai commented Dec 24, 2018 •

edited

Loading

mstoodle Jan 10, 2019 •

edited

Loading

mstoodle commented Jan 10, 2019 •

edited

Loading