This is an archive of the discontinued LLVM Phabricator instance.

[JITLink][Orc] Add MemoryMapper interface with InProcess implementation
ClosedPublic

Authored by argentite on Jun 10 2022, 5:47 AM.

Details

Summary

MemoryMapper class takes care of cross-process and in-process address space
reservation, mapping, transferring content and applying protections.

Implementations of this class can support different ways to do this such
as using shared memory, transferring memory contents over EPC or just
mapping memory in the same process (InProcessMemoryMapper).

Diff Detail

Event Timeline

argentite created this revision.Jun 10 2022, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2022, 5:47 AM
lhames added a subscriber: lhames.Jun 10 2022, 9:23 AM

What we're looking for is a breakdown in responsibilities where:
(1) The MappingJITLinkMemoryManager is responsible for object layout (of blocks into segments), and heap management (subject to constraints that the Mapper will describe).
(2) The Mapper is responsible for address space, physical memory management (and content transfer), and recording and applying actions.

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
29–33

Mapping shouldn't be associated with a local address: It just represents the reserved range in the executor.

I don't think we need a separate type for this -- An ExecutorAddrRange should be ideal.

36

typos: offset, relative.

39–45

The current struct has actions associated with the segment. One allocation may contain multiple segments, but only one list of actions -- the actions are associated with the allocation, rather than any particular segment.

Initialize will also need the address of the working memory for each segment.

I think you probably want something more like:

struct AllocInfo {
  struct SegInfo {
    ExecutorAddrDiff Offset;
    const char *WorkingMem;
    size_t ContentSize;
    size_t ZeroFillSize;
    unsigned Prot;
  };
  ExecutorAddr MappingBase;
  std::vector<SegInfo> Segments;
  shared::AllocActions Actions;
};
52

We'll want a prepare method in here. This is just responsible for setting up working memory, so it can be synchronous:

prepare :: [ (executor-addr, content-size, prot) ] -> [ char * ]

58–59

Initialize just needs to return a unique address associated with the recorded dealloc actions in the executor, so under the changes proposed above this would become:

virtual ExecutorAddr initialize(AllocInfo &AI, OnInitializedFunction OnInitialized) = 0;

We'll use this ExecutorAddr as the value for the FinalizedAlloc that we return from the memory manager.

64–65

deinitialize should just take the ExecutorAddr that was returned from initialize, so:

virtual void `deinitialize(ExecutorAddr A, OnDeInitialized) = 0;
argentite updated this revision to Diff 436145.Jun 11 2022, 8:35 AM
argentite edited the summary of this revision. (Show Details)

Introduced prepare() and Allocation to contain multiple segments and actions

Today was busier than anticipated -- I've read the new patch, but haven't had a chance to try it out yet.

I've added some notes on the prepare method, and will try it out tomorrow morning.

llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp
39–42

The protection change should still be part of initialize, and prepare can drop the Protections argument.

The prepare method's job is just to associate _working memory_ with the given _executor address_ in such a way that we can follow-up with the content transfer during initialize. For InProcessMapper the executor address doubles as the address of the working memory, so prepare really does reduce to nothing but a char* cast.

For something like EPCMemoryMapper it'll be more interesting: prepare will malloc some memory (or more likely allocate it using the LinkGraphs allocator), and record the association between that allocated working memory and the target address. Then in initialize we'll issue an EPC call to copy the content from working memory over to the executor.

argentite updated this revision to Diff 436233.Jun 12 2022, 9:36 AM

Removed unnecessary protection argument

Finally got to check this out -- I've added some more feedback comments.

This is looking great. I think it's ready for public review, with the aim that we land it in-tree this week.

What do you think? Are you happy with it?

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
49

I think you can probably simplify this further and just return a char * -- I don't know of any situation where our working memory allocation could fail.

71–97

Synchronous versions of the APIs? Very interesting. I've been assuming that Mapper would _only_ have MappingJITLinkMemoryManager as a client, and that we'd eventually end up moving the interface into that class (or making it a concept and having MappingJITLinkMemoryManager take a template argument), but maybe it's worth having as a utility in its own right.

Did you have a use-case in mind?

llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp
66–74

I think you can use runFinalizeActions from AllocationActions.h here.

89–99

I think you can use runDeallocActions from AllocationActions.h here.

133–137

This looks like runDeallocActions again.

llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp
32

Could you add some comments to this function describing the steps? E.g.

// Define counters -- these will be incremented/decremented in
// finalize/dealloc actions to verify that the actions are run.
38–39

You can write this as:

auto PageSize = cantFail(sys::Process::getPageSize());

That will auto-unwrap the Expected for you.

argentite updated this revision to Diff 436748.Jun 14 2022, 5:37 AM
argentite marked 13 inline comments as done.

Simplified prepare return type
Used runFinalizeActions and runDeallocActions instead of reimplementing
Changed some mutex lock scopes

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
71–97

Not really. it just felt nicer when writing the tests. We can remove them.

argentite published this revision for review.Jun 14 2022, 8:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 8:29 PM
lhames added inline comments.
llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
71–97

I think you should sink them into the testcase as free functions for now. We can lift them back into the class later if we decide we want to encourage this kind of direct use of the Mapper interface.

llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp
32

Just noticed -- this test should be renamed too. :)

argentite updated this revision to Diff 437427.Jun 15 2022, 8:00 PM
argentite marked 3 inline comments as done.

Moved synchronous functions to test and renamed test

Overall, I think this looks pretty good! I didn't follow the design discussions closely, so please bare with me in case some of my questions are dumb :-) In a way it makes sure other people will understand your code as well.

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
61

Nit: typo deinitialization

63

In which case do we have multiple ExecutorAddr here? initialize() only returns a single one -- can it run multiple times for a single MemoryMapper? If this is the case, it may deserve a test case and/or a comment.

If we keep the std::vector I guess it should be passed as const std::vector<ExecutorAddr> & -- or is there a design reason for mutability here?

69

The base class doesn't deal with processes. And release() is the inverse operation for reserve() right? Then we might want the comment to reflect that and say something like Release address space in the executor process.

78

What is the reason for the using MemoryMapper::... statements when we only have one base class? My first thought was that it's a hint for another overload in the base class, but it doesn't seem to be the case.

92

Maybe we can add a comment for implict vs. explicit deinit/release somewhere? This seems to be specific for the InProcessMemoryMapper. It took me a while to figure out that the destructor reimplements them (instead of calling) for the implicit case.

llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp
51

Base = AI.MappingBase + Segment.Offset

69

Can we add a comment that explains the special role of MinAddr in regard to allocation actions here? Thanks

llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp
145

Alloc1 is testing: explicit deinit + implicit release
Alloc2 is testing: implicit deinit + implicit release

I think they are both good to have. However, the explicit release case has no coverage yet.

argentite updated this revision to Diff 437550.Jun 16 2022, 8:12 AM
argentite marked 6 inline comments as done.

Explicit release test and minor fixes

argentite marked an inline comment as not done.Jun 16 2022, 8:14 AM
argentite added inline comments.
llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
63

As I understand the a single MemoryMapper should live for the whole duration of JITLink use. When terminating, we can pass the results of multiple initialize to a single deinitialize to save RPC overhead. I have made the vectors const for both deinitialize and release.

78

Ah there were synchronous overloads that we moved to unit test. I forgot to delete them.

92

The plan is to have some kind of implicit deinitialization in all MemoryMappers. The release() and deinitialize() are there for cases where we want to release early without terminating the program. Do you think anything specific that we should mention?

llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp
145

I have added a test for explicit deinit + explicit release but note that explicit release does not implicitly deinitialize because allocations are not directly tied to a reservation. (Should it?)

argentite updated this revision to Diff 437858.Jun 17 2022, 5:25 AM
argentite marked an inline comment as done.

release() will implicitly deinitialize() all suballocations to maintain consistency

lhames accepted this revision.Jun 19 2022, 10:00 PM

A couple of small comments, but otherwise LGTM. I think this is ready to land in tree.

Stefan — did you have any other feedback?

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
56

I think we want to keep the contract on the address returned by initialize loose:

/// Returns a unique address identifying the allocation. This address should
/// be passed to deinitialize to run deallocation actions (and reset permissions
/// where possible).

The InProcessAllocator uses the lowest allocated address as a key into a DenseMap, but you could imagine schemes that return a pointer to a bookkeeping struct instead.

63

I think that an ArrayRef would do — that way any array-like list of addresses would work.

This revision is now accepted and ready to land.Jun 19 2022, 10:00 PM
sgraenitz accepted this revision.Jun 20 2022, 1:37 AM

LGTM. Let's go and land this once Lang's two latest comment got addressed.

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
92

With the comments on deinitialize()/release() I think it's ok now.

argentite updated this revision to Diff 438373.Jun 20 2022, 6:32 AM
argentite marked 2 inline comments as done.

Used ArrayRef instead of const std::vector& and relaxed initialize() return address restriction

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
56

Should we reset permissions? And if so what should we reset to? PROT_NONE?

sgraenitz added inline comments.Jun 21 2022, 3:32 AM
llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
56

No, I don't think so. None of the other uses of releaseMappedMemory() in LLVM does it. And if it's relevant for a use-case it could always be done in a dealloc action right?

This revision was landed with ongoing or failed builds.Jun 21 2022, 4:45 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 21 2022, 5:51 AM

This breaks the build on Windows: http://45.33.8.238/win/60587/step_4.txt

Please take a look, and revert for now if it takes a while to fix.

Thanks for taking care of reverting.

The issue is that MSVC's std::future implementation requires default constructible types. It's a known issue, but I had not thought about it during the review. When defining a std::promise we have to use MSVCPError and MSVCPExpected<T> instead of Error and Expected<T> respectively. I will fix it and re-apply the patch.