[PATCH] SectionMemoryManager is not a JITMemoryManager

Filip Pizlo fpizlo at apple.com
Thu May 9 15:33:36 PDT 2013


On May 9, 2013, at 3:22 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:

> Hi Filip,
> 
> The more I think about this the more I think the side-by-side set[MCJIT|JIT]MemoryManager functions is tolerable since we mean to break them apart long term anyway.
> 
> If you want to put together an updated patch with the intermediate MCJITMemoryManager class I think we can proceed with this.

Hi Andrew!

Sorry for silence on my end - I've been in WebKit land for a bit.

The more I think about it, the more I believe that introducing the intermediate MCJITMemoryManager class would just not be helpful.  This class wouldn't do anything right now.  If we find a reason for this class in the future, then we can add it in the future.  It won't be a difficult refactoring to pull off.

OTOH, I totally agree that the notion of "MCJITMemoryManager" is great for the C API - since there we actually have a need for future-proofing of the API.  I'm working on a follow-up patch that exposes RTDyldMemoryManager via the C API, and there I will just call it "MCJITMemoryManager" instead.  But I don't think that the C++ API needs that kind of future-proofing.

Does that sound reasonable?

Once I get back to this patch, I'll address some of the other feedback I got (like removing some unnecessary casts from tests) and post a new patch.  If you have no objection, I'll do that without the MCJITMemoryManager intermediate class.

> 
> Is this something you were hoping to see in the 3.3 release or can it just go into trunk?

I don't need it in 3.3.  So trunk is fine.  (WebKit is building from trunk right now, and will be, for the foreseeable future.)

-Filip

> 
> -Andy
> 
> -----Original Message-----
> From: Filip Pizlo [mailto:fpizlo at apple.com] 
> Sent: Monday, May 06, 2013 5:10 PM
> To: Kaylor, Andrew
> Cc: llvm-commits at cs.uiuc.edu; Rafael EspĂ­ndola; Jim Grosbach (grosbach at apple.com)
> Subject: Re: [PATCH] SectionMemoryManager is not a JITMemoryManager
> 
> Thanks for the feedback!
> 
> I really like the MCJITMemoryManager intermediate class idea. 
> 
> I'm ok with pausing on this for a day or so. But I'm also really excited about getting to the point where WebKit can supply a MM to the MCJIT, and if I did that now it would be *ugly*. Hence my desire to simplify.
> 
> -Filip
> 
> On May 6, 2013, at 4:41 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:
> 
>> I definitely like the patch and this is the direction I was hoping to see this move.  I only have a couple of concerns.
>> 
>> First, I don't like the propagation of the name "RTDyldMemoryManager" at the EngineBuilder level.  It's quite a mouthful, and it exposes an implementation detail.  The RuntimeDyld component is designed to be able to function alone outside MCJIT, and that's why it has its own memory manager class.  The fact that MCJIT is implemented in terms of that memory manager class is something that isn't likely to change, but it's still an artifact of the implementation.  I'm not convinced we want people using MCJIT to be thinking about RuntimeDyld.
>> 
>> I'm not sure I have an alternative that I like better to offer, but I don't want to do this incorrectly.  The layering of components in this area has been tricky to keep straight in the past, and I want to be careful not to take any steps in the wrong direction.  I don't want to introduce a method to setRTDyldMemoryManager when what we're really setting the memory manager for is MCJIT.  There may come a time when MCJIT needs to do some additional things to manage memory that aren't connected to RuntimeDyld.
>> 
>> I'm inclined to think that it would be best to introduce an MCJITMemoryManager class that did nothing but wrap RTDyldMemoryManager for interface purposes and call the new method EngineBuilder::setMCJITMemoryManager().
>> 
>> At the same time, I'm not thrilled with having an interface with setJITMemoryManager and setMCJITMemoryManager methods co-existing.  The underlying implementations might be getting cleaner, but the EngineBuilder is getting uglier and more confusing.  When I first read this patch, I thought it wasn't going to work with things like LLDB that use MCJIT but have memory managers implemented using the DefaultJITMemoryManager.  I see on closer inspection that I was wrong, but my first impression is a hint as to the resultant usability of the EngineBuilder interface, which has already been a source of confusion.
>> 
>> I'd like to see some variant of this patch go in, but I'd like to give it a few days to stew and solicit input from other interested parties in the meantime.
>> 
>> -Andy
>> 
>> -----Original Message-----
>> From: Rafael EspĂ­ndola [mailto:rafael.espindola at gmail.com] 
>> Sent: Monday, May 06, 2013 12:50 PM
>> To: Filip Pizlo
>> Cc: llvm-commits at cs.uiuc.edu; Kaylor, Andrew
>> Subject: Re: [PATCH] SectionMemoryManager is not a JITMemoryManager
>> 
>> I love this patch, but I am new to MCJIT too. Andrew, what do you think?
>> 
>> On 4 May 2013 15:37, Filip Pizlo <fpizlo at apple.com> wrote:
>>> This patch cleans up the relationship between RTDyldMemoryManager, JITMemoryManager, and EngineBuilder.
>>> 
>>> As I understand it, JITMemoryManager is meant to go away at some point, and RTDyldMemoryManager is not meant to be predestined to have to support all of the things that JITMemoryManager had; it may have some of the same things, but also maybe some new things, and probably not all of the old things.
>>> 
>>> But right now EngineBuilder doesn't know anything about RTDyldMemoryManager and assumes that JITMemoryManager is the way memory is managed in both MCJIT and the old JIT.  This means that if you want to pass an RTDyldMemoryManager to the MCJIT via the EngineBuilder API, you have to actually create a JITMemoryManager and stub out a *bunch* of methods.  SectionMemoryManager does this, for example: SectionMemoryManager is a subtype of JITMemoryManager that cannot actually be used as a JITMemoryManager, except passing it through EngineBuilder so that MCJIT can use it as an RTDyldMemoryManager. :-/
>>> 
>>> This patch attempts to fix the problem:
>>> 
>>> - EngineBuilder now knows that RTDyldMemoryManager and JITMemoryManager are different but similar things. It knows that if you set a JITMemoryManager, then it can be used for the MCJIT.  It knows that an RTDyldMemoryManager can also be used for the MCJIT, but cannot be used for old JIT.  It will ensure that you cannot set both at the same time since that would be confusing (setting one overrides the other).
>>> 
>>> - SectionMemoryManager is no longer a subtype of JITMemoryManager, and no longer has a ton of stub unimplemented methods.
>>> 
>>> - lli and various tests are changed to know that a SectionMemoryManager is not a JITMemoryManager.
>>> 
>>> This is a change to the C++ API, and I actually don't know that the policies on this are!  But I hope that this is something that we can change, since it kills some gnarly code and concentrates whatever little ugliness remains into EngineBuilder (it's only ugly because it has to know that there are two kinds of ways of saying that you want to manager memory yourself).
>>> 
>>> -Filip
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130509/06d14d25/attachment.html>


More information about the llvm-commits mailing list