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

yvvan abandoned this revision. yvvan added a comment. @zturner The fact that so many call-sites decide what to do is pretty error-prone. There was already at least one issue when this flag was not properly set by some of the call-sites. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D549

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

zturner added a comment. Assuming this patch were to go in as-is (which it probably won't, based on the feedback, but let's just pretend), that would avoid having to explicitly update how many callsites? What I'm wondering is, how hard would it be to just update the call-sites? It looks like w

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

I don’t think we really need this. isn’t Ilya’s solution in the other patch already sufficient? On Mon, Dec 3, 2018 at 7:34 AM Ivan Donchevskii via Phabricator < revi...@reviews.llvm.org> wrote: > yvvan added a comment. > > @ilya-biryukov > > Hm. What about another way around? - We have user inclu

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

yvvan added a comment. @ilya-biryukov Hm. What about another way around? - We have user include paths (-I) and report them to the filesystem. This means that we have specific paths under which nothing can be mmaped and everything else can be. In particular cases we can also report -isystem the

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

ilya-biryukov added a comment. In D54995#1316476 , @yvvan wrote: > I don't think removing the flag is a good idea since I can easily assume > cases when user wants mmap and is ready to encounter locks. In our case it > can be an IDE option which behavior

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

yvvan added a comment. In D54995#1316457 , @ilya-biryukov wrote: > In D54995#1316437 , @yvvan wrote: > > > Ok, no global option. > > Why not placing your VolatileFSProvider in clang so that libclang could > > you

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

ilya-biryukov added a comment. In D54995#1316437 , @yvvan wrote: > Ok, no global option. > Why not placing your VolatileFSProvider in clang so that libclang could you > it too? Would be happy to, will need to figure out what to do with PCH and PCM file

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

yvvan planned changes to this revision. yvvan added a comment. Ok, no global option. Why not placing your VolatileFSProvider in clang so that libclang could you it too? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54995/new/ https://reviews.llvm.org/D54995 _

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

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

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

yvvan marked an inline comment as done. yvvan added inline comments. Comment at: lib/Support/MemoryBuffer.cpp:42 +static bool MemoryMappingEnabled = true; + lebedev.ri wrote: > Such global flags are a bad idea in general, and really not great in LLVM's > case.

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

lebedev.ri added inline comments. Comment at: lib/Support/MemoryBuffer.cpp:42 +static bool MemoryMappingEnabled = true; + 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 a

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

yvvan updated this revision to Diff 176318. yvvan retitled this revision from "[MemoryBuffer] By default assume that all files are volatile to prevent unintended file locks" to "[MemoryBuffer] Add the setter to be able to force disabled mmap". yvvan edited the summary of this revision. yvvan adde