[PATCH] D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

It seems there are still cases where memory-mapping should be preferred, even on Windows, specifically:

- PCH files produced by libclang: they are huge, loading them in memory is wasteful and they can clearly be used
- (maybe?) PCM (precompiled module) files: they are huge, however can be produced by the user's buildsystem. Locking them might be bad if we don't control the buildsystem, but the memory savings are attractive.
- other binary input files?

At least the PCM files seem to be low-hanging fruits, we should try to take advantage of them.



================
Comment at: lib/Support/MemoryBuffer.cpp:42
 
+static bool MemoryMappingEnabled = true;
+
----------------
yvvan wrote:
> lebedev.ri wrote:
> > Such global flags are a bad idea in general, and really not great in LLVM's case.
> > The editor would set it for "it's" llvm, but that will also affect the LLVM that is used by e.g. mesa.
> > 
> Oh no, don't mention mesa. The proper client should never share it's LLVM layer with mesa. We already got issues with versions incompatibility and the only good solution is to link llvm statically inside client. Otherwise mesa causes the mess anyways.
> 
> Also I expect this setter to be used only on Windows.
> 
> Of course there's another solution is that Ilya proposed but it's only for clangd there and is bigger in code size. And it can still allow bugs if somebody uses MemoryBuffer not through the FileSystem class.
+1, please don't add global flags, there are lots of reasons to avoid them.
Moreover, adding this without removing the current approach (isVolatile) is twice as bad - we now have many ways to indicate the files cannot be memory-mapped.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54995/new/

https://reviews.llvm.org/D54995