r194250 - clang-format: Write files atomically

[Daniel Jasper]

On Fri, Nov 8, 2013 at 1:09 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 08/11/2013 20:34, Daniel Jasper wrote:
> > The patch itself looks good (I don't know why we didn't do that in the
> > first place).
> >
> >
> > On Fri, Nov 8, 2013 at 12:07 AM, Alp Toker <alp at nuanti.com
> > <mailto:alp at nuanti.com>> wrote:
> >
> >     Author: alp
> >     Date: Fri Nov  8 02:07:19 2013
> >     New Revision: 194250
> >
> >     URL: http://llvm.org/viewvc/llvm-project?rev=194250&view=rev
> >     Log:
> >     clang-format: Write files atomically
> >
> >     Switch clang-format over to Rewriter::overwriteChangedFiles().
> >
> >     The previous implementation was attempting to stream back directly
> >     to the
> >     original file and failing if it was already memory mapped by
> >     MemoryBuffer,
> >     an operation unsupported by Windows.
> >
> >     MemoryBuffer generally mmaps files larger than the physical page
> >     size so
> >     this will have been difficult to reproduce consistently.
> >
> >     This change also reduces flicker in code editors and IDEs on all
> >     platforms
> >     when reformatting in-place.
> >
> >
> > I think it is generally a bad idea for IDEs to use in-place
> > reformatting, but ok ..
>
> This was referring to running clang-format on the commandline while you
> have files open in XCode. From time to time it went wrong, especially
> with unsaved changes.
>
> >
> >     Note that other incorrect uses of MemoryBuffer exist in LLVM/clang
> and
> >     will need a similar fix.
> >
> >     A test should be added for Windows when libFormat performance
> >     issues are
> >     fixed (it takes longer than a day to format a 1MB file at present!)
> >
> >
> > Is it the write that takes so long (i.e. fixed with this patch)?
> >
> > If not, can the file be shared (or changed enough so that it can be
> > shared)?
>
> This happened with various internal sources pasted together, but I think
> it can also be reproduced with sqlite.c:
>
>   http://www.sqlite.org/2013/sqlite-amalgamation-3080100.zip
>
> (My RewriteBuffer patch doesn't help with this one.)
>

Thanks for the test case. The problem here is the (relatively) new logic we
have to format complex #if/#else branches correctly even if they cut
through statements. The file is not only large, but also has deeply nested
#if/#else combinations (6+ levels). With this feature turned off, the file
formats in ~10sec, which IMO is reasonable for a file of that size.

There are two routes forward:
a) There are quite a few low-hanging fruits in optimizing our current
approach. Quite probably those will be sufficient.
b) Turn off this formatting feature for very large files as it only
improves corner cases (possibly not even happing in this file).

We'll look into it (not top priority at the moment, though).

Cheers,
Daniel

For now I'm continuing to track down the MemoryBuffer issues in other
> parts of LLVM making the same mistake as clang-format.cpp. So won't be
> looking at this performance issue for a while -- feel free to pick it up..
>
> Alp.
>
> >
> >     Modified:
> >         cfe/trunk/tools/clang-format/ClangFormat.cpp
> >
> >     Modified: cfe/trunk/tools/clang-format/ClangFormat.cpp
> >     URL:
> >
> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/ClangFormat.cpp?rev=194250&r1=194249&r2=194250&view=diff
> >
> ==============================================================================
> >     --- cfe/trunk/tools/clang-format/ClangFormat.cpp (original)
> >     +++ cfe/trunk/tools/clang-format/ClangFormat.cpp Fri Nov  8
> >     02:07:19 2013
> >     @@ -213,18 +213,8 @@ static bool format(std::string FileName)
> >          Rewriter Rewrite(Sources, LangOptions());
> >          tooling::applyAllReplacements(Replaces, Rewrite);
> >          if (Inplace) {
> >     -      if (Replaces.size() == 0)
> >     -        return false; // Nothing changed, don't touch the file.
> >     -
> >     -      std::string ErrorInfo;
> >     -      llvm::raw_fd_ostream FileStream(FileName.c_str(), ErrorInfo,
> >     -                                      llvm::sys::fs::F_Binary);
> >     -      if (!ErrorInfo.empty()) {
> >     -        llvm::errs() << "Error while writing file: " << ErrorInfo
> >     << "\n";
> >     +      if (Rewrite.overwriteChangedFiles())
> >              return true;
> >     -      }
> >     -      Rewrite.getEditBuffer(ID).write(FileStream);
> >     -      FileStream.flush();
> >          } else {
> >            if (Cursor.getNumOccurrences() != 0)
> >              outs() << "{ \"Cursor\": " << tooling::shiftedCodePosition(
> >
> >
> >     _______________________________________________
> >     cfe-commits mailing list
> >     cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
> >     http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131108/915ad0c0/attachment.html>