Fix performance problem in long-running SectionMemoryManagers

[loladiro]

Summary

Without this patch, the memory manager would call mprotect on every memory region it ever allocated whenever it wanted to finalize memory (i.e. not just the ones it just allocated). This caused terrible performance problems for long running memory managers. In one particular compile heavy julia benchmark, we were spending 50% of time in mprotect if running under MCJIT. Fix this by splitting
allocated memory blocks into those on which memory permissions have been set and those on which
they haven't and only running mprotect on the latter.

[reames]

include/llvm/ExecutionEngine/SectionMemoryManager.h

87 ↗

(On Diff #35701)

Comment explaining purpose?

lib/ExecutionEngine/SectionMemoryManager.cpp

169 ↗

(On Diff #35701)

I think you need to extend the destructor to handle the case where the memory manager is destructed before finalization.

[lhames]

Pending addressing of Philip's comment this looks great to me. Thanks Keno!

[loladiro]

lib/ExecutionEngine/SectionMemoryManager.cpp

169 ↗

(On Diff #35701)

Thanks! I guess we also need to release the FreeMem blocks in here then.

[reames]

lib/ExecutionEngine/SectionMemoryManager.cpp

169 ↗

(On Diff #35701)

Nope. Be careful with this. FreeMem appears to point inside regions tracked by AllocatedMem

[loladiro]

lib/ExecutionEngine/SectionMemoryManager.cpp

169 ↗

(On Diff #35701)

I see, thanks!

[loladiro]

Addresses review comments. @reames let me know if you agree with the wording of the explanatory
comment.

[reames]

LGTM

lib/ExecutionEngine/SectionMemoryManager.cpp

143 ↗

(On Diff #36145)

very minor: I'd put the clear immediately after each append.

[loladiro]

Thanks for the quick review. Also, while looking at this again, I noticed, we're setting RX permissions on RO memory. Does that sound right? Seems like that memory should be executable? Isn't that the whole point of splitting this?

[reames]

I assume you meant that we are applying execute permissions to the read
only memory and probably shouldn't be? If so, I tend to agree.
However, I'd have to study the code much more carefully to understand
the implications. There may be an issue with the relocations or something.

Philip

llvm/trunk/include/llvm/ExecutionEngine/SectionMemoryManager.h

public:
   /// explicit cache flush, otherwise JIT code manipulations (like resolved
   /// relocations) will get to the data cache but not to the instruction cache.
   ///
   /// This method is called from finalizeMemory.
   virtual void invalidateInstructionCache();
 
 private:
   struct MemoryGroup {
-      SmallVector<sys::MemoryBlock, 16> AllocatedMem;
+      // PendingMem contains all allocated memory blocks
+      // which have not yet had their permissions set. Note
+      // that this tracks memory blocks that have been given to
+      // this memory manager by the system, not those
+      // given out to the user. In particular, the memory manager
+      // will give out subblocks of these MemoryBlocks in response
+      // to user requests. We track which subblocks have not beeen
+      // given out yet in `FreeMem`.
+      SmallVector<sys::MemoryBlock, 16> PendingMem;
       SmallVector<sys::MemoryBlock, 16> FreeMem;
+
+      // All allocated memory blocks that have had their permissions
+      // set (i.e. that have been finalized). Because of this, we may
+      // not give out subblocks of this memory to the user anymore,
+      // even if those subblocks have not been previously given out.
+      SmallVector<sys::MemoryBlock, 16> AllocatedMem;
+
       sys::MemoryBlock Near;
   };
 
   uint8_t *allocateSection(MemoryGroup &MemGroup, uintptr_t Size,
                            unsigned Alignment);
 
   std::error_code applyMemoryGroupPermissions(MemoryGroup &MemGroup,
                                               unsigned Permissions);

llvm/trunk/lib/ExecutionEngine/SectionMemoryManager.cpp

uint8_t *SectionMemoryManager::allocateSection(MemoryGroup &MemGroup,
   if (ec) {
     // FIXME: Add error propagation to the interface.
     return nullptr;
   }
 
   // Save this address as the basis for our next request
   MemGroup.Near = MB;
 
-  MemGroup.AllocatedMem.push_back(MB);
+  MemGroup.PendingMem.push_back(MB);
   Addr = (uintptr_t)MB.base();
   uintptr_t EndOfBlock = Addr + MB.size();
 
   // Align the address.
   Addr = (Addr + Alignment - 1) & ~(uintptr_t)(Alignment - 1);
 
   // The allocateMappedMemory may allocate much more memory than we need. In
   // this case, we store the unused memory as a free memory block.
bool SectionMemoryManager::finalizeMemory(std::string *ErrMsg)
 
   // Read-write data memory already has the correct permissions
 
   // Some platforms with separate data cache and instruction cache require
   // explicit cache flush, otherwise JIT code manipulations (like resolved
   // relocations) will get to the data cache but not to the instruction cache.
   invalidateInstructionCache();
 
+  // Now, remember that we have successfully applied the permissions to avoid
+  // having to apply them again.
+  CodeMem.AllocatedMem.append(CodeMem.PendingMem.begin(),CodeMem.PendingMem.end());
+  CodeMem.PendingMem.clear();
+
+  RODataMem.AllocatedMem.append(RODataMem.PendingMem.begin(),RODataMem.PendingMem.end());
+  RODataMem.PendingMem.clear();
+
   return false;
 }
 
 std::error_code
 SectionMemoryManager::applyMemoryGroupPermissions(MemoryGroup &MemGroup,
                                                   unsigned Permissions) {
 
-  for (sys::MemoryBlock &MB : MemGroup.AllocatedMem)
+  for (sys::MemoryBlock &MB : MemGroup.PendingMem)
     if (std::error_code EC = sys::Memory::protectMappedMemory(MB, Permissions))
       return EC;
 
   return std::error_code();
 }
 
 void SectionMemoryManager::invalidateInstructionCache() {
-  for (sys::MemoryBlock &Block : CodeMem.AllocatedMem)
+  for (sys::MemoryBlock &Block : CodeMem.PendingMem)
     sys::Memory::InvalidateInstructionCache(Block.base(), Block.size());
 }
 
 SectionMemoryManager::~SectionMemoryManager() {
-  for (MemoryGroup *Group : {&CodeMem, &RWDataMem, &RODataMem})
+  for (MemoryGroup *Group : {&CodeMem, &RWDataMem, &RODataMem}) {
     for (sys::MemoryBlock &Block : Group->AllocatedMem)
       sys::Memory::releaseMappedMemory(Block);
+    for (sys::MemoryBlock &Block : Group->PendingMem)
+      sys::Memory::releaseMappedMemory(Block);
+  }
 }
 
 } // namespace llvm