[READY] Cache include paths

[bstaletic]

Since @davits said he won't be able to finish #873, I've updated his PR according to the review comments.

All credits go to @davits for actually doing all the work here.

---

This change is 

[bstaletic]

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved.

---

ycmd/completers/cpp/flags.py, line 678 at r2 (raw file):

                      '-isystem': ( include_paths, True ) }
    if _ShouldAllowWinStyleFlags( user_flags ):
      include_flags[ '/I' ] = ( include_paths, False )

@puremourning mentioned somehow making the use of the include flags at the top of the file.
Considering the flags at the top of the file are somewhat different, what would be the best approach?

---

ycmd/completers/cpp/flags.py, line 682 at r2 (raw file):

    try:
      it = iter( user_flags )
      for user_flag in it:

@puremourning This shouldn't use iterkeys because it iterates over a list, not over dictionary keys.

---

ycmd/completers/cpp/include_cache.py, line 115 at r2 (raw file):

        del self._cache[ path ]
      else:
        includes = cache_entry[ 1 ]

@puremourning We could probably update the whole directory entry instead of just deleting it, but...

We still need to do most of the work we are already doing, yet the code would get less readable.
In case you believe this is worthwhile, I'll gladly do it. After all, I'm not the one having troubles with metadata performance.

---

Comments from Reviewable

[bstaletic]

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cpp/flags.py, line 678 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

@puremourning mentioned somehow making the use of the include flags at the top of the file.
Considering the flags at the top of the file are somewhat different, what would be the best approach?

The reason we're not going "full-hog" and caching everything right now is that the lines above make it easy to test what works the best.

@puremourning Once you say what works best, we could even drop tupples and maybe unconditionally use cache.

---

ycmd/completers/cpp/include_cache.py, line 115 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

@puremourning We could probably update the whole directory entry instead of just deleting it, but...

We still need to do most of the work we are already doing, yet the code would get less readable.
In case you believe this is worthwhile, I'll gladly do it. After all, I'm not the one having troubles with metadata performance.

Actually, if we change _AddToCache( self, path, includes ) to _AddToCache( self, path, includes, mtime = None ), then this can be rewritten like:

  if mtime > cache_entry[ 0 ]:
    _AddToCache( path, self._ListInclude( path ), mtime )
  includes = cache_entry[ 1]

This looks like it should work.
Am I missing something?

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #919 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #919      +/-   ##
=========================================
+ Coverage   97.08%   97.1%   +0.01%     
=========================================
  Files          88      89       +1     
  Lines        6904    6969      +65     
=========================================
+ Hits         6703    6767      +64     
- Misses        201     202       +1

[bstaletic]

Coverage can be improved, but before I get to writing more tests, do we want/need ClearIncludeCache completer subcommand?

---

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[davits]

@bstaletic thanks a lot for continuing to work on this :)

---

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bstaletic]

@davits No problem. You have done most of the work though. Also, since I can't test it, if you have the time give it another test.

[davits]

Tested, Acceleration confirmed!

---

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bstaletic]

Thanks for testing! 👍 Did you test it with only system header caching? How does it behave if you let it cache all headers?

…

On Feb 15, 2018 4:29 PM, "Davit Samvelyan" ***@***.***> wrote: Tested, Acceleration confirmed! ------------------------------ Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/valloric/ycmd/919#-:-L5P4AxBCN3rUVDw1MY5:b-lqjhgo>* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#919 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHbkU4SiyKPXNVDZ9VcPVCr3RacMJ-dwks5tVE1PgaJpZM4R-cOC> .

[davits]

In my case there is no much difference, since almost all include paths are given with '-isystem', there are only 2 paths given with '-I'. I think it wouldn't hurt to also cache '-I' flags, that folders rarely change as well, in my case at least.

---

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bstaletic]

Great, your include flags are the opposite of @puremourning's. I'll wait for his test results and then update the PR appropriately. Once again, thanks for testing.

…

On Feb 15, 2018 4:46 PM, "Davit Samvelyan" ***@***.***> wrote: In my case there is no much difference, since almost all include paths are given with '-isystem', there are only 2 paths given with '-I'. I think it wouldn't hurt to also cache '-I' flags, that folders rarely change as well, in my case at least. ------------------------------ Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/valloric/ycmd/919#-:-L5P6qQHf1vh016n_1IP:b-dg2wxp>* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#919 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHbkUwws6fPnqXinNMhA1t6gZizzYzCMks5tVFFzgaJpZM4R-cOC> .

[bstaletic]

I've pushed the tests. This is ready, unless someone has more to say.

---

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cpp/flags.py, line 678 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

The reason we're not going "full-hog" and caching everything right now is that the lines above make it easy to test what works the best.

@puremourning Once you say what works best, we could even drop tupples and maybe unconditionally use cache.

Okay, we are definitely caching everything.

---

Comments from Reviewable

[bstaletic]

I squashed the commits.
Since two people have said this works and nobody complained about anything I'll mark this as ready.

---

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[zzbot]

☔ The latest upstream changes (presumably #974) made this pull request unmergeable. Please resolve the merge conflicts.

[bstaletic]

Conflicts resolved.

[micbou]

Reviewed 1 of 3 files at r2, 1 of 2 files at r4, 4 of 6 files at r5, 1 of 1 files at r6, 3 of 3 files at r7, 3 of 3 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

---

ycmd/completers/cpp/flags.py, line 669 at r6 (raw file):

def UserIncludePaths( user_flags, filename ):
  """
  Returns a tupple ( quoted_include_paths, include_paths )

tuple

---

ycmd/completers/cpp/include_cache.py, line 51 at r8 (raw file):

  Helper class for combining include completion candidates from
  several include paths.
  slef._includes is a dictionary whose keys are

self.

---

ycmd/completers/cpp/include_cache.py, line 76 at r8 (raw file):

  Holds a dictionary representing the include path cache.
  Dictionary keys are the include path directories.
  Dictionary values are tupples whose first object

tuples

---

ycmd/completers/cpp/include_cache.py, line 82 at r8 (raw file):

  def __init__( self ):
    self._cache = {}

I think a lock is needed for this variable as it can be simultaneously updated by multiple threads in the Clang completer.

---

ycmd/completers/cpp/include_cache.py, line 107 at r8 (raw file):

    if cache_entry:
      mtime = _GetModificationTime( path )
      if mtime > cache_entry[ 'mtime' ]:

This raises a TypeError exception if mtime is None.

---

ycmd/completers/general/filename_completer.py, line 39 at r7 (raw file):

# This mapping is also used for the #include completion.
# We have option N3, because when using Qt with specific include paths
# configuration both file & dir entries can exist.

This comment is misleading. It makes the reader thinks it's specific to Qt. It's not. We should simply say that an entry can be simultaneously a file and a directory when completing include statements.

---

ycmd/tests/clang/include_cache_test.py, line 129 at r8 (raw file):

                   } ) ) )

    sleep( 10 )

sleep( 2 ) (or even sleep( 1.001 )) should be enough.

---

Comments from Reviewable

[bstaletic]

Review status: 2 of 6 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

ycmd/completers/cpp/flags.py, line 669 at r6 (raw file):

Previously, micbou wrote…

tuple

Done.

---

ycmd/completers/cpp/include_cache.py, line 51 at r8 (raw file):

Previously, micbou wrote…

self.

Done.

---

ycmd/completers/cpp/include_cache.py, line 76 at r8 (raw file):

Previously, micbou wrote…

tuples

Done.

---

ycmd/completers/cpp/include_cache.py, line 82 at r8 (raw file):

Previously, micbou wrote…

I think a lock is needed for this variable as it can be simultaneously updated by multiple threads in the Clang completer.

Done, I believe. Feel free to tell me if I messed something up.

---

ycmd/completers/cpp/include_cache.py, line 107 at r8 (raw file):

Previously, micbou wrote…

This raises a TypeError exception if mtime is None.

I changed the _GetModificationTime() to return 0 if getmtime() fails. That should allow us to avoid another try/except.

---

ycmd/completers/general/filename_completer.py, line 39 at r7 (raw file):

Previously, micbou wrote…

This comment is misleading. It makes the reader thinks it's specific to Qt. It's not. We should simply say that an entry can be simultaneously a file and a directory when completing include statements.

Done.

---

ycmd/tests/clang/include_cache_test.py, line 129 at r8 (raw file):

Previously, micbou wrote…

sleep( 2 ) (or even sleep( 1.001 )) should be enough.

Done.

---

Comments from Reviewable

[micbou]

Reviewed 4 of 4 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

---

ycmd/tests/clang/include_cache_test.py, line 44 at r10 (raw file):

def GetCache_NotCached_DirInaccessible_test():

Why are the name of the tests starting with GetCache? Wouldn't IncludeCache be a better choice?

---

Comments from Reviewable

[bstaletic]

Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/tests/clang/include_cache_test.py, line 44 at r10 (raw file):

Previously, micbou wrote…

Why are the name of the tests starting with GetCache? Wouldn't IncludeCache be a better choice?

You're right. Fixed.

---

Comments from Reviewable

[micbou]

---

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[puremourning]

Reviewed 2 of 6 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/completers/cpp/flags.py, line 673 at r5 (raw file):

  quoted_include_paths is a list of include paths that are only suitable for
  quoted include statement.
  include_paths is a list of include paths that can be used for angle bracked

bracket or perhaps bracketed

---

ycmd/completers/cpp/include_cache.py, line 115 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Actually, if we change _AddToCache( self, path, includes ) to _AddToCache( self, path, includes, mtime = None ), then this can be rewritten like:

  if mtime > cache_entry[ 0 ]:
    _AddToCache( path, self._ListInclude( path ), mtime )
  includes = cache_entry[ 1]

This looks like it should work.
Am I missing something?

seems ok. We're not reading the time multiple times, but I think the other path means we don't set includes anymore

---

ycmd/completers/cpp/include_cache.py, line 36 at r5 (raw file):

class IncludeEntry( object ):

maybe use a named tuple ? saves on some code. meh. no need to change.

---

ycmd/completers/cpp/include_cache.py, line 1 at r11 (raw file):

# Copyright (C) 2017 Davit Samvelyan [email protected]

I suppose as you have made changes to this, you can claim some copyright ownership as well?

---

ycmd/completers/cpp/include_cache.py, line 98 at r11 (raw file):

  def _AddToCache( self, path, includes, mtime = None ):
    if not mtime:

not ? or "is not None" ? Do we want "falsy" to trigger here?

---

ycmd/completers/cpp/include_cache.py, line 108 at r11 (raw file):

    includes = None
    with self._cache_lock:
      cache_entry = self._cache.get( path )

do we mutate cache_entry? if so we provably need to hold the lock until we have made any modifications. Otherwise there is a race condition where you acquire a pointer to the entry, then someone else does the same, then you update the list, then someone else updates the list.

Probably not the end of the world if it happens (we just update the cache 2x rather than blocking). Hmm 🤔

---

ycmd/completers/cpp/include_cache.py, line 112 at r11 (raw file):

      mtime = _GetModificationTime( path )
      if mtime > cache_entry[ 'mtime' ]:
        self._AddToCache( path, self._ListIncludes( path ), mtime )

includes doesn't get set in this branch

---

ycmd/completers/cpp/include_cache.py, line 123 at r11 (raw file):

      names = os.listdir( path )
    except OSError:
      return []

any reason not to _logger.exception this ? if not, let's comment that "the directory no longer exists or is no longer accessible, it must be empty" or something/

---

Comments from Reviewable

[puremourning]

Reviewed 1 of 6 files at r5, 1 of 3 files at r8, 3 of 4 files at r9, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

Comments from Reviewable

[bstaletic]

Review status: 4 of 6 files reviewed at latest revision, 8 unresolved discussions.

---

ycmd/completers/cpp/flags.py, line 673 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

bracket or perhaps bracketed

Done.
Opted for bracketed.

---

ycmd/completers/cpp/include_cache.py, line 115 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

seems ok. We're not reading the time multiple times, but I think the other path means we don't set includes anymore

Yeah, that's what I ended up doing.

---

ycmd/completers/cpp/include_cache.py, line 36 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

maybe use a named tuple ? saves on some code. meh. no need to change.

Done.

---

ycmd/completers/cpp/include_cache.py, line 1 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I suppose as you have made changes to this, you can claim some copyright ownership as well?

Done.

---

ycmd/completers/cpp/include_cache.py, line 98 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

not ? or "is not None" ? Do we want "falsy" to trigger here?

Upon reading this again I was sure you were right.
Once I switched to is not None the cache completely broke and I have no idea why.

In this method mtime can have three values: None, 0, or something valid.

• None should mean the mtime wasn't passed to the method.
• 0 should mean we couldn't read the mtime.
• Valid value should be stored.

---

ycmd/completers/cpp/include_cache.py, line 108 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

do we mutate cache_entry? if so we provably need to hold the lock until we have made any modifications. Otherwise there is a race condition where you acquire a pointer to the entry, then someone else does the same, then you update the list, then someone else updates the list.

Probably not the end of the world if it happens (we just update the cache 2x rather than blocking). Hmm 🤔

We potentially mutate the cache in case the new mtime is newer than the stored one.
I tend to think that one should avoid locks as much as possible, but feel free to prove me wrong and tell me that we should block.

---

ycmd/completers/cpp/include_cache.py, line 112 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

includes doesn't get set in this branch

Done.

---

ycmd/completers/cpp/include_cache.py, line 123 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

any reason not to _logger.exception this ? if not, let's comment that "the directory no longer exists or is no longer accessible, it must be empty" or something/

Done.

---

Comments from Reviewable

[puremourning]

with a few comments

---

Review status: 4 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cpp/include_cache.py, line 1 at r11 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Done.

I think Synopsis is the company @davits works for, so it should be kept attached.

---

ycmd/completers/cpp/include_cache.py, line 108 at r11 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

We potentially mutate the cache in case the new mtime is newer than the stored one.
I tend to think that one should avoid locks as much as possible, but feel free to prove me wrong and tell me that we should block.

I'm sure it's fine. Worst case is we stash the dir 2x in a very unlikely race condition

---

ycmd/completers/cpp/include_cache.py, line 112 at r11 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Done.

Is there a way to add a test for this? I guess it is a bit of a gap.

---

ycmd/completers/cpp/include_cache.py, line 44 at r12 (raw file):

entry_type is an integer indicating whether the candidate is a
'File', 'Dir' or both (See EXTRA_INFO_MAP in filename_completer). """
IncludeEntry = namedtuple( 'IncludeEntry', 'name entry_type' )

I feel like field names should be a list. This is totally legit, but I find the way named tuple allows whitespace or comma separated strings unusual! Anyway, no action required.

---

Comments from Reviewable

[puremourning]

Reviewed 2 of 2 files at r12.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[bstaletic]

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

ycmd/completers/cpp/include_cache.py, line 1 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think Synopsis is the company @davits works for, so it should be kept attached.

I had no idea. Fixed.

---

ycmd/completers/cpp/include_cache.py, line 112 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Is there a way to add a test for this? I guess it is a bit of a gap.

There is a IncludeCache_Cached_NewMtime_test() which makes sure the correct thing is placed in the cache in this branch.
I'm not sure how to write a test that makes sure includes is always set.

---

ycmd/completers/cpp/include_cache.py, line 44 at r12 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I feel like field names should be a list. This is totally legit, but I find the way named tuple allows whitespace or comma separated strings unusual! Anyway, no action required.

Done.

This is what happens when you implement something base on the first blog post you find. I didn't know until now that python had named tuples.

---

Comments from Reviewable

[puremourning]

---

Review status: 5 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cpp/include_cache.py, line 112 at r11 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

There is a IncludeCache_Cached_NewMtime_test() which makes sure the correct thing is placed in the cache in this branch.
I'm not sure how to write a test that makes sure includes is always set.

Check the results ?

---

ycmd/completers/cpp/include_cache.py, line 44 at r12 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Done.

This is what happens when you implement something base on the first blog post you find. I didn't know until now that python had named tuples.

hehe

---

ycmd/tests/clang/include_cache_test.py, line 135 at r13 (raw file):

      bar_file.write( 'bar' )

    new_includes = include_cache.GetIncludes( tmp_dir )

Rereading the code, I think this was succeeding because _GetCached was returning None, then GetIncludes was re-cacheing the directory.

We could in theory prove it with mocking, but it's probably not worth it.

---

Comments from Reviewable

[puremourning]

Reviewed 1 of 1 files at r13.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[bstaletic]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/cpp/include_cache.py, line 112 at r11 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Check the results ?

The results of GetIncludes is checked. (Note that there is no leading underscore here.)
Are you thinking about basically unit testing _GetIncludes?

---

ycmd/tests/clang/include_cache_test.py, line 135 at r13 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Rereading the code, I think this was succeeding because _GetCached was returning None, then GetIncludes was re-cacheing the directory.

We could in theory prove it with mocking, but it's probably not worth it.

Yup, you're completely right, though I'm not sure what to mock.

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cpp/include_cache.py, line 112 at r11 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

The results of GetIncludes is checked. (Note that there is no leading underscore here.)
Are you thinking about basically unit testing _GetIncludes?

Sorry I meant to delete that comment, after adding the other one about the test.

---

Comments from Reviewable

[micbou]

Reviewed 2 of 2 files at r12, 1 of 1 files at r13.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

ycmd/completers/cpp/include_cache.py, line 123 at r11 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Done.

_logger.exception should be used so that we know where the error comes from in the logs.

---

ycmd/completers/cpp/include_cache.py, line 142 at r13 (raw file):

  except OSError:
    _logger.debug( 'The directory {} no longer exists or is inaccessible.'
                   .format( path ) )

Same comment.

---

Comments from Reviewable

[bstaletic]

Review status: 5 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/cpp/include_cache.py, line 123 at r11 (raw file):

Previously, micbou wrote…

_logger.exception should be used so that we know where the error comes from in the logs.

Done.

---

ycmd/completers/cpp/include_cache.py, line 142 at r13 (raw file):

Previously, micbou wrote…

Same comment.

Done.

---

Comments from Reviewable

[micbou]

Reviewed 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

ycmd/completers/cpp/include_cache.py, line 123 at r11 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Done.

Sorry, I missed this but we shouldn't format the string when logging. Instead, we should do:

_logger.exception( 'The directory %s no longer exists or is inaccessible.', path )

Also, I think the message should give more context e.g. Cannot list entries for include path %s.

---

ycmd/completers/cpp/include_cache.py, line 142 at r13 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Done.

The message here could be Cannot get modification time for include path %s.

---

Comments from Reviewable

[bstaletic]

Review status: 5 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/cpp/include_cache.py, line 123 at r11 (raw file):

Previously, micbou wrote…

Sorry, I missed this but we shouldn't format the string when logging. Instead, we should do:

_logger.exception( 'The directory %s no longer exists or is inaccessible.', path )

Also, I think the message should give more context e.g. Cannot list entries for include path %s.

Done.

---

ycmd/completers/cpp/include_cache.py, line 142 at r13 (raw file):

Previously, micbou wrote…

The message here could be Cannot get modification time for include path %s.

Done.

---

Comments from Reviewable

[micbou]

Nice work.

@zzbot r+

---

Reviewed 1 of 1 files at r15.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[zzbot]

📌 Commit 55d3997 has been approved by micbou

[zzbot]

⌛ Testing commit 55d3997 with merge 1c19003...

[zzbot]

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 1c19003 to master...