[all-commits] [llvm/llvm-project] b500c4: [clangd] Don't mmap source files on all platforms ...

[Sam McCall via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: b500c49cd4f81f067cda721049cb1fd72a5e7bf5
      https://github.com/llvm/llvm-project/commit/b500c49cd4f81f067cda721049cb1fd72a5e7bf5
  Author: Sam McCall <sam.mccall at gmail.com>
  Date:   2020-01-29 (Wed, 29 Jan 2020)

  Changed paths:
    M clang-tools-extra/clangd/FSProvider.cpp
    M clang-tools-extra/clangd/FSProvider.h

  Log Message:
  -----------
  [clangd] Don't mmap source files on all platforms --> don't crash on git checkout

Summary:
Previously we mmapped on unix and not on windows: on windows mmap takes
an exclusive lock on the file and prevents the user saving changes!

The failure mode on linux is a bit more subtle: if the file is changed on disk
but the SourceManager sticks around, then subsequent operations on the
SourceManager will fail as invariants are violated (e.g. null-termination).

This commonly manifests as crashes after switching git branches with many files
open in clangd.

Nominally mmap is for performance here, and we should be willing to give some
up to stop crashing. Measurements on my system (linux+desktop+SSD) at least
show no measurable regression on an a fairly IO-heavy workload: drop disk caches,
open SemaOverload.cpp, wait for first diagnostics.

for i in `seq 100`; do
  for variant in mmap volatile; do
    echo 3 | sudo tee /proc/sys/vm/drop_caches
    /usr/bin/time --append --quiet -o ~/timings -f "%C %E" \
    bin/clangd.$variant -sync -background-index=0 < /tmp/mirror > /dev/null
  done
done

bin/clangd.mmap -sync -background-index=0 0:07.60
bin/clangd.volatile -sync -background-index=0 0:07.89
bin/clangd.mmap -sync -background-index=0 0:07.44
bin/clangd.volatile -sync -background-index=0 0:07.89
bin/clangd.mmap -sync -background-index=0 0:07.42
bin/clangd.volatile -sync -background-index=0 0:07.50
bin/clangd.mmap -sync -background-index=0 0:07.90
bin/clangd.volatile -sync -background-index=0 0:07.53
bin/clangd.mmap -sync -background-index=0 0:07.64
bin/clangd.volatile -sync -background-index=0 0:07.55
bin/clangd.mmap -sync -background-index=0 0:07.75
bin/clangd.volatile -sync -background-index=0 0:07.47
bin/clangd.mmap -sync -background-index=0 0:07.90
bin/clangd.volatile -sync -background-index=0 0:07.50
bin/clangd.mmap -sync -background-index=0 0:07.81
bin/clangd.volatile -sync -background-index=0 0:07.95
bin/clangd.mmap -sync -background-index=0 0:07.55
bin/clangd.volatile -sync -background-index=0 0:07.65
bin/clangd.mmap -sync -background-index=0 0:08.15
bin/clangd.volatile -sync -background-index=0 0:07.54
bin/clangd.mmap -sync -background-index=0 0:07.78
bin/clangd.volatile -sync -background-index=0 0:07.61
bin/clangd.mmap -sync -background-index=0 0:07.78
bin/clangd.volatile -sync -background-index=0 0:07.55
bin/clangd.mmap -sync -background-index=0 0:07.41
bin/clangd.volatile -sync -background-index=0 0:07.40
bin/clangd.mmap -sync -background-index=0 0:07.54
bin/clangd.volatile -sync -background-index=0 0:07.42
bin/clangd.mmap -sync -background-index=0 0:07.45
bin/clangd.volatile -sync -background-index=0 0:07.49
bin/clangd.mmap -sync -background-index=0 0:07.95
bin/clangd.volatile -sync -background-index=0 0:07.66
bin/clangd.mmap -sync -background-index=0 0:08.04

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D73617