[llvm-commits] [PATCH] Add mapped_file_region for read/write mapped files.

[Nick Kledzik]

On Aug 6, 2012, at 3:37 PM, Michael Spencer wrote:
> On Mon, Aug 6, 2012 at 2:58 PM, Nick Kledzik <kledzik at apple.com> wrote:
>>> +  mapped_file_region(const Twine &path,
>>> +                     mapmode mode,
>>> +                     uint64_t length,
>>> +                     uint64_t offset,
>>> +                     error_code &ec);
>> 
>> Something about constructors that also return an error_code sits wrong with me.  In the error case, the constructor succeeds but you are left with a half constructed object.
> 
> I don't like it either, but the alternatives don't allow RAII. And all
> of them (except exceptions) require checking a value to make sure the
> returned mapped_file_region is valid.
I know, we've had other discussions about this issue.  I keep hoping someone will have a clever solution.

> 
>> By having the error_code at the end, you can now no longer use default values for the length and offset parameters.  Your test case has to pass 0, 0 for them which is non-obvious what is happening.  Since mapping the whole file is common, it would be nice if that case could just be called without the offset and length parameters.
> 
> It's easy to just move ec to be the first parameter.
It is nice have return/out parameters at the end...

> 
>> To support unix and Windows which have different state requirements, you had to create another struct (mapstate) to encapsulate that.  So now there are two structs to manage.
> 
> mapstate is an internal implementation detail. Users don't know
> anything about mapstate.
My point was not about clients.  It was that the implementation was complicated by the extra class.

>> I would think it solve the above three issues and be cleaner to define an abstract base class and use a factory method for creation.  Then the unix and Windows implementations have their own subclasses with the ivars they each need, and the factory method (in the .inc file) just calls the constructor on the concrete subclass.  The interface in FileSystem.h would just be the abstract interface:
>> 
>> class mapped_file_region {
>> public;
>>        mapped_file_region() LLVM_EQ_DELETE;
>>        mapped_file_region(mapped_file_region&) LLVM_EQ_DELETE;
>>        mapped_file_region &operator =(mapped_file_region&) LLVM_EQ_DELETE;
>> 
>>        static mapped_file_region* make(const Twine &path, mapmode mode, uint64_t length=0, uint64_t offset=0);
>> 
>>        virtual error_code   error() const;
>>        virtual mapmode    flags() const;
>>        virtual uint64_t        size() const;
>>        virtual char            *data() const;
>> };
> 
> I have two issues with this. The first is that mapped_file_region uses
> RAII and move semantics to manage its lifetime, this forces users to
> use delete or wrap it in an OwningPtr.
Yes, the factory method idea precludes a client have a local variable that will automatically be destroyed at the end of the function or object.  


> The second is that there is no
> reason to have virtual functions when only one implementation can ever
> be used.
I think the more general rule is don't use indirection what unnecessary.  But your implementation used indirection by having just one ivar that points to the real ivars.  So, both have an indirection.  The virtual
methods are just more obvious as indirection.

The point of the indirection is to hide that the unix and Windows implementation are radically different and require different amounts of state.  Maybe we should just push the indirection in the pre-processor.  That is,
in FileSystem.h have:
 
	class mapped_file_region {
	…
	void               *View;
	mapmode     Mode;
	uint64_t		Size;
	#if LLVM_ON_WIN32
	int                   FD;
	HANDLE       File;
	HANDLE       FileMapping;
	#endif
	…

So the extra ivars show up when building for Windows but not for unix.  Then there is no need for indirection! 
         

> 
> This also still has the problem of partially constructed objects.
Yes, that is why I had a separate error() method to check for failure.

A variation on this is to have the constructor just assign ivars and not actually try to do the mapping, and thus cannot fail.  Instead we add a method:
     error_code map();
which does the mapping and can fail.  

I think the import part is that all the methods that operate on the object fire an assertion if called on a "partially constructed" object.  

-Nick


> 
>> 
>>> +  int flags = mode == mapped_file_region::readwrite ? MAP_SHARED : MAP_PRIVATE;
>> I also found this hard to parse.  How about:
>> 
>> int flags = ((mode == mapped_file_region::readwrite) ? MAP_SHARED : MAP_PRIVATE);
> 
> ok.
> 
>> -Nick
> 
> - Michael Spencer
> 
>> 
>> 
>> On Aug 6, 2012, at 2:00 PM, Michael Spencer wrote:
>>> The attached patch adds mapped_file_region based on
>>> boost::iostreams::mapped_file. This is a replacement for
>>> map_file_pages/unmap_file_pages to work properly on Windows. This can
>>> also be used by MemoryBuffer to finally have memory mapped files on
>>> Windows.
>>> 
>>> The reason the previous APIs do not work for Windows is that there are
>>> 3 different handles that need to be kept track of to properly unmap a
>>> region. For munmap there is only 1.
>>> 
>>> - Michael Spencer
>>> <0001-PathV2-Add-mapped_file_region.-Implementation-for-Wi.patch>
>>