From: Ben Peart Date: Wed, 29 Aug 2018 21:42:56 GMT Subject: Re: [PATCH v2 2/3] read-cache: load cache extensions on worker thread Message-ID: <606bb7e9-0d58-ec86-6a3c-8a123679f9f4@gmail.com> In-Reply-To: On 8/29/2018 1:12 PM, Junio C Hamano wrote: > Ben Peart writes: > >> This is possible because the current extensions don't access the cache >> entries in the index_state structure so are OK that they don't all exist >> yet. >> >> The CACHE_EXT_TREE, CACHE_EXT_RESOLVE_UNDO, and CACHE_EXT_UNTRACKED >> extensions don't even get a pointer to the index so don't have access to the >> cache entries. >> >> CACHE_EXT_LINK only uses the index_state to initialize the split index. >> CACHE_EXT_FSMONITOR only uses the index_state to save the fsmonitor last >> update and dirty flags. > > Good to see such an analysis here. Once we define an extension > section, which requires us to have the cache entries before > populating it, this scheme would falls down, of course, but the > extension mechanism is all about protecting ourselves from the > future changes, so we'd at least need a good feel for how we read an > unknown extension from the future with the current code. Perhaps > just like the main cache entries were pre-scanned to apportion them > to worker threads, we can pre-scan the sections and compare them > with a white-list built into our binary before deciding that it is > safe to read them in parallel (and otherwise, we ask the last thread > for reading extensions to wait until the workers that read the main > index all return)? > Yes, when we add a new extension that requires the cache entries to exist and be parsed, we will need to add a mechanism to ensure that happens for that extension. I agree a white list is probably the right way to deal with it. Until we have that need, it would just add unnecessary complexity so I think we should wait till it is actually needed. There isn't any change in behavior with unknown extensions and this patch. If an unknown extension exists it will just get ignored and reported as an "unknown extension" or "die" if it is marked as "required." I'll fix the rest of your suggestions - thanks for the close review.