[PATCH 0/3] Avoid confusing messages from new index extensions (Re: [PATCH v8 0/7] speed up index load through parallelization)
Hi, Ben Peart wrote: > Ben Peart (6): > read-cache: clean up casting and byte decoding > eoie: add End of Index Entry (EOIE) extension > config: add new index.threads config setting > read-cache: load cache extensions on a worker thread > ieot: add Index Entry Offset Table (IEOT) extension > read-cache: load cache entries on worker threads I love this, but when deploying it I ran into a problem. How about these patches? Thanks, Jonathan Nieder (3): eoie: default to not writing EOIE section ieot: default to not writing IEOT section index: do not warn about unrecognized extensions Documentation/config.txt | 14 ++ read-cache.c | 24 +--- t/t1700-split-index.sh | 11 +++ 3 files changed, 42 insertions(+), 7 deletions(-)
Re: [PATCH v8 0/7] speed up index load through parallelization
fixup! IEOT error messages
Enable localizing new error messages and improve the error message for
invalid IEOT extension sizes.
Signed-off-by: Ben Peart
---
read-cache.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 7acc2c86f4..f9fa6a7979 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3480,7 +3480,7 @@ static struct index_entry_offset_table
*read_ieot_extension(const char *mmap, si
/* validate the version is IEOT_VERSION */
ext_version = get_be32(index);
if (ext_version != IEOT_VERSION) {
- error("invalid IEOT version %d", ext_version);
+ error(_("invalid IEOT version %d"), ext_version);
return NULL;
}
index += sizeof(uint32_t);
@@ -3488,7 +3488,7 @@ static struct index_entry_offset_table
*read_ieot_extension(const char *mmap, si
/* extension size - version bytes / bytes per entry */
nr = (extsize - sizeof(uint32_t)) / (sizeof(uint32_t) +
sizeof(uint32_t));
if (!nr) {
- error("invalid number of IEOT entries %d", nr);
+ error(_("invalid IEOT extension size %d"), extsize);
return NULL;
}
ieot = xmalloc(sizeof(struct index_entry_offset_table)
--
2.18.0.windows.1
On 10/14/2018 8:28 AM, Duy Nguyen wrote:
On Wed, Oct 10, 2018 at 5:59 PM Ben Peart wrote:
@@ -3460,14 +3479,18 @@ static struct index_entry_offset_table
*read_ieot_extension(const char *mmap, si
/* validate the version is IEOT_VERSION */
ext_version = get_be32(index);
- if (ext_version != IEOT_VERSION)
+ if (ext_version != IEOT_VERSION) {
+ error("invalid IEOT version %d", ext_version);
Please wrap this string in _() so that it can be translated.
return NULL;
+ }
index += sizeof(uint32_t);
/* extension size - version bytes / bytes per entry */
nr = (extsize - sizeof(uint32_t)) / (sizeof(uint32_t) +
sizeof(uint32_t));
- if (!nr)
+ if (!nr) {
+ error("invalid number of IEOT entries %d", nr);
Ditto. And reporting extsize may be more useful than nr, which we know
is zero, but we don't know why it's calculated zero unless we know
extsize.
Re: [PATCH v8 0/7] speed up index load through parallelization
On Wed, Oct 10, 2018 at 5:59 PM Ben Peart wrote:
> @@ -3460,14 +3479,18 @@ static struct index_entry_offset_table
> *read_ieot_extension(const char *mmap, si
>
> /* validate the version is IEOT_VERSION */
> ext_version = get_be32(index);
> - if (ext_version != IEOT_VERSION)
> + if (ext_version != IEOT_VERSION) {
> + error("invalid IEOT version %d", ext_version);
Please wrap this string in _() so that it can be translated.
>return NULL;
> + }
> index += sizeof(uint32_t);
>
> /* extension size - version bytes / bytes per entry */
> nr = (extsize - sizeof(uint32_t)) / (sizeof(uint32_t) +
> sizeof(uint32_t));
> - if (!nr)
> + if (!nr) {
> + error("invalid number of IEOT entries %d", nr);
Ditto. And reporting extsize may be more useful than nr, which we know
is zero, but we don't know why it's calculated zero unless we know
extsize.
--
Duy
Re: [PATCH v8 0/7] speed up index load through parallelization
Ben Peart writes: > From: Ben Peart > > Fixed issues identified in review the most impactful probably being plugging > some leaks and improved error handling. Also added better error messages > and some code cleanup to code I'd touched. > > The biggest change in the interdiff is the impact of renaming ieot_offset to > ieot_start and ieot_work to ieot_blocks in hopes of making it easier to read > and understand the code. Thanks, I think this one is ready to be in 'next' and any further tweaks can be done incrementally.
[PATCH v8 0/7] speed up index load through parallelization
From: Ben Peart
Fixed issues identified in review the most impactful probably being plugging
some leaks and improved error handling. Also added better error messages
and some code cleanup to code I'd touched.
The biggest change in the interdiff is the impact of renaming ieot_offset to
ieot_start and ieot_work to ieot_blocks in hopes of making it easier to read
and understand the code.
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/6caa0bac46
Checkout: git fetch https://github.com/benpeart/git read-index-multithread-v8
&& git checkout 6caa0bac46
### Interdiff (v7..v8):
diff --git a/read-cache.c b/read-cache.c
index 14402a0738..7acc2c86f4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1901,7 +1901,7 @@ struct index_entry_offset
struct index_entry_offset_table
{
int nr;
- struct index_entry_offset entries[0];
+ struct index_entry_offset entries[FLEX_ARRAY];
};
#ifndef NO_PTHREADS
@@ -1935,9 +1935,7 @@ static void *load_index_extensions(void *_data)
* extension name (4-byte) and section length
* in 4-byte network byte order.
*/
- uint32_t extsize;
- memcpy(, p->mmap + src_offset + 4, 4);
- extsize = ntohl(extsize);
+ uint32_t extsize = get_be32(p->mmap + src_offset + 4);
if (read_index_extension(p->istate,
p->mmap + src_offset,
p->mmap + src_offset + 8,
@@ -2015,8 +2013,8 @@ struct load_cache_entries_thread_data
int offset;
const char *mmap;
struct index_entry_offset_table *ieot;
- int ieot_offset;/* starting index into the ieot array */
- int ieot_work; /* count of ieot entries to process */
+ int ieot_start; /* starting index into the ieot array */
+ int ieot_blocks;/* count of ieot entries to process */
unsigned long consumed; /* return # of bytes in index file processed */
};
@@ -2030,8 +2028,9 @@ static void *load_cache_entries_thread(void *_data)
int i;
/* iterate across all ieot blocks assigned to this thread */
- for (i = p->ieot_offset; i < p->ieot_offset + p->ieot_work; i++) {
- p->consumed += load_cache_entry_block(p->istate,
p->ce_mem_pool, p->offset, p->ieot->entries[i].nr, p->mmap,
p->ieot->entries[i].offset, NULL);
+ for (i = p->ieot_start; i < p->ieot_start + p->ieot_blocks; i++) {
+ p->consumed += load_cache_entry_block(p->istate, p->ce_mem_pool,
+ p->offset, p->ieot->entries[i].nr, p->mmap,
p->ieot->entries[i].offset, NULL);
p->offset += p->ieot->entries[i].nr;
}
return NULL;
@@ -2040,48 +2039,45 @@ static void *load_cache_entries_thread(void *_data)
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)
{
- int i, offset, ieot_work, ieot_offset, err;
+ int i, offset, ieot_blocks, ieot_start, err;
struct load_cache_entries_thread_data *data;
unsigned long consumed = 0;
- int nr;
/* a little sanity checking */
if (istate->name_hash_initialized)
BUG("the name hash isn't thread safe");
mem_pool_init(>ce_mem_pool, 0);
- data = xcalloc(nr_threads, sizeof(struct
load_cache_entries_thread_data));
/* ensure we have no more threads than we have blocks to process */
if (nr_threads > ieot->nr)
nr_threads = ieot->nr;
- data = xcalloc(nr_threads, sizeof(struct
load_cache_entries_thread_data));
+ data = xcalloc(nr_threads, sizeof(*data));
- offset = ieot_offset = 0;
- ieot_work = DIV_ROUND_UP(ieot->nr, nr_threads);
+ offset = ieot_start = 0;
+ ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);
for (i = 0; i < nr_threads; i++) {
struct load_cache_entries_thread_data *p = [i];
- int j;
+ int nr, j;
- if (ieot_offset + ieot_work > ieot->nr)
- ieot_work = ieot->nr - ieot_offset;
+ if (ieot_start + ieot_blocks > ieot->nr)
+ ieot_blocks = ieot->nr - ieot_start;
p->istate = istate;
p->offset = offset;
p->mmap = mmap;
p->ieot = ieot;
- p->ieot_offset = ieot_offset;
- p->ieot_work = ieot_work;
+ p->ieot_start = ieot_start;
+ p->ieot_blocks = ieot_blocks;
/* create a mem_pool for each thread */
nr = 0;
- for (j = p->ieot_offset; j < p->ieot_offset + p->ieot_work; j++)
+ for (j = p->ieot_start; j < p->ieot_start + p->ieot_blocks; j++)
