From: Ben Peart Date: Mon, 22 Oct 2018 14:40:00 GMT Subject: Re: [PATCH v8 7/7] read-cache: load cache entries on worker threads Message-ID: <72c8938c-7ffa-a384-55dc-09d90d9cf7f8@gmail.com> In-Reply-To: On 10/21/2018 10:14 PM, Junio C Hamano wrote: > Jeff King writes: > >> On Wed, Oct 10, 2018 at 11:59:38AM -0400, Ben Peart wrote: >> >>> +static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size, >>> + unsigned long src_offset, int nr_threads, struct index_entry_offset_table *ieot) >> >> The src_offset parameter isn't used in this function. >> >> In early versions of the series, it was used to feed the p->start_offset >> field of each load_cache_entries_thread_data. But after the switch to >> ieot, we don't, and instead feed p->ieot_start. But we always begin that >> at 0. >> >> Is that right (and we can drop the parameter), or should this logic: >> >>> + offset = ieot_start = 0; >>> + ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads); >>> + for (i = 0; i < nr_threads; i++) { >>> [...] >> >> be starting at src_offset instead of 0? > > I think "offset" has nothing to do with the offset into the mmapped > region of memory. It is an integer index into a (virtual) array > that is a concatenation of ieot->entries[].entries[], and it is > correct to count from zero. The value taken from that array using > the index is used to compute the offset into the mmapped region. > > Unlike load_all_cache_entries() called from the other side of the > same if() statement in the same caller, this does not depend on the > fact that the first index entry in the mmapped region appears > immediately after the index-file header. It goes from the offsets > into the file that are recorded in the entry offset table that is an > index extension, so the sizeof(*hdr) that initializes src_offset is > not used by the codepath. > > The number of bytes consumed, i.e. its return value from the > function, is not really used, either, as the caller does not use > src_offset for anything other than updating it with "+=" and passing > it to this function (which does not use it) when it calls this > function (i.e. when ieot extension exists--and by definition when > that extension exists extension_offset is not 0, so we do not make > the final load_index_extensions() call in the caller that uses > src_offset). > Thanks for discovering/analyzing this. You're right, I missed removing this when we switched from a single offset to an array of offsets via the IEOT. I'll send a patch to fix both issues shortly.