On 10/21/2018 10:14 PM, Junio C Hamano wrote:
Jeff King <peff@xxxxxxxx> 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.
