read-cache: speed up index load through parallelization
Re: [PATCH v7 7/7] read-cache: load cache entries on worker threads
In reply to Ben Peartread-cache.c static void *load_index_extensions(void *_data) · line 1960
Quoted 6 lines · Ben Peart
read-cache.c +6
-0+/* +* A helper function that will load the specified range of cache entries +* from the memory mapped file and add them to the given index. +*/ +static unsigned long load_cache_entry_block(struct index_state *istate, + struct mem_pool *ce_mem_pool, int offset, int nr, const char *mmap,
Please use unsigned long for offset (here and in the thread_data struct). We should use off_t instead, but that's out of scope. At least keep offset type consistent in here.
Quoted 1 line
+ unsigned long
start_offset, const structcache_entry*previous_ce)
I don't think you want to pass previous_ce in. You always pass NULL anyway. And if this function is about loading a block (i.e. at block boundary) then initial previous_ce _must_ be NULL or things break horribly.
read-cache.c static void *load_index_extensions(void *_data) · line 2018
Quoted 13 lines
read-cache.c +9
-0@@ -1959,20 +2007,125 @@ static void *load_index_extensions(void *_data)#define THREAD_COST (10000) +struct load_cache_entries_thread_data +{ +pthread_t pthread; +struct index_state *istate; +struct mem_pool *ce_mem_pool; +int offset; +const char *mmap; +struct index_entry_offset_table *ieot; +int ieot_offset;/* starting index into the ieot array */
If it's an index, maybe just name it ieot_index and we can get rid of the comment.
read-cache.c static void *load_index_extensions(void *_data) · line 2019
Quoted 1 line
read-cache.c +1
-0+int ieot_work;/* count of ieot entries to process */
Maybe instead of saving the whole "ieot" table here. Add
struct index_entry_offset *blocks;
which points to the starting block for this thread and rename that mysterious (to me) ieot_work to nr_blocks. The thread will have access from blocks[0] to blocks[nr_blocks - 1]
Quoted 5 lines
read-cache.c +5
-0+unsigned long consumed;/* return # of bytes in index file processed */ +}; + +/* +* A thread proc to run the load_cache_entries() computation
read-cache.c static void *load_index_extensions(void *_data) · line 2034
Quoted 10 lines
read-cache.c +10
-0+* across multiple background threads. +*/ +static void *load_cache_entries_thread (void *_data) +{ +struct load_cache_entries_thread_data *p = _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 );
Please wrap this long line.
Quoted 9 lines
read-cache.c +9
-0+ p->offset += p->ieot->entries[i].nr; + } +return NULL ; +} + +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;
read-cache.c static void *load_index_extensions(void *_data) · line 2053
Quoted 10 lines
read-cache.c +10
-0+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 (&istate->ce_mem_pool,0 ); + data =xcalloc (nr_threads,sizeof (struct load_cache_entries_thread_data ));
we normally use sizeof(*data) instead of sizeof(struct ...)
read-cache.c static void *load_index_extensions(void *_data) · line 2058
Quoted 5 lines
read-cache.c +5
-0+ +/* 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 ));
eh.. reallocate the same "data"?
Quoted 9 lines
read-cache.c +9
-0+ + offset = ieot_offset =0 ; + ieot_work =DIV_ROUND_UP (ieot->nr, nr_threads); +for (i =0 ; i < nr_threads; i++) { +struct load_cache_entries_thread_data *p = &data[i]; +int j; + +if (ieot_offset + ieot_work > ieot->nr) + ieot_work = ieot->nr - ieot_offset;
read-cache.c static void *load_index_extensions(void *_data) · line 2077
Quoted 10 lines
read-cache.c +10
-0+ + p->istate = istate; + p->offset = offset; + p->mmap = mmap; + p->ieot = ieot; + p->ieot_offset = ieot_offset; + p->ieot_work = ieot_work; + +/* create a mem_pool for each thread */ + nr =0 ;
Since nr is only used in this for loop. Declare it in this scope instead of declaring it for the whole function.
read-cache.c static void *load_index_extensions(void *_data) · line 2087
Quoted 10 lines
read-cache.c +10
-0+for (j = p->ieot_offset; j < p->ieot_offset + p->ieot_work; j++) + nr += p->ieot->entries[j].nr; +if (istate->version ==4 ) { +mem_pool_init (&p->ce_mem_pool, +estimate_cache_size_from_compressed (nr)); + } +else { +mem_pool_init (&p->ce_mem_pool, +estimate_cache_size (mmap_size, nr)); + }
Maybe keep this mem_pool_init code inside load_cache_entries_thread(), similar to how you do it for load_cache_entries_thread(). It's mostly to keep this loop shorter to see (and understand), of course parallelizing this mem_pool_init() is just noise.
read-cache.c static void *load_index_extensions(void *_data) · line 2095
Quoted 8 lines
read-cache.c +8
-0+ + err =pthread_create (&p->pthread,NULL , load_cache_entries_thread, p); +if (err) +die (_ ("unable to create load_cache_entries thread: %s" ),strerror (err)); + +/* increment by the number of cache entries in the ieot block being processed */ +for (j =0 ; j < ieot_work; j++) + offset += ieot->entries[ieot_offset + j].nr;
I wonder if it makes things simpler if you store cache_entry _index_ in entrie[] array instead of storing the number of entries. You can easily calculate nr then by doing entries[i].index - entries[i-1].index. And you can count multiple blocks the same way, without looping like this.
Quoted 40 lines
read-cache.c +22
-3+ ieot_offset += ieot_work; + } + +for (i =0 ; i < nr_threads; i++) { +struct load_cache_entries_thread_data *p = &data[i]; + + err =pthread_join (p->pthread,NULL ); +if (err) +die (_ ("unable to join load_cache_entries thread: %s" ),strerror (err)); +mem_pool_combine (istate->ce_mem_pool, p->ce_mem_pool); + consumed += p->consumed; + } + +free (data); + +return consumed; +} +#endif +/* remember to discard_cache() before reading a different cache! */ int do_read_index (struct index_state *istate,const char *path,int must_exist) { -int fd, i; +int fd;struct stat st;unsigned long src_offset;const struct cache_header *hdr;const char *mmap;size_t mmap_size; -const struct cache_entry *previous_ce =NULL ;struct load_index_extensions p;size_t extension_offset =0 ;#ifndef NO_PTHREADS -int nr_threads; +int nr_threads, cpus; +struct index_entry_offset_table *ieot =NULL ;#endif if (istate->initialized)
read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2170
Quoted 4 lines
read-cache.c +1
-0@@ -2014,10 +2167,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) p.mmap = mmap; p.mmap_size = mmap_size; + src_offset =sizeof (*hdr);
OK we've been doing this since forever, sizeof(struct cache_header) probably does not have extra padding on any supported platform.
Quoted 15 lines
read-cache.c +0
-10@@ -2032,29 +2193,22 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) nr_threads--; } } -#endif - -if (istate->version ==4 ) { -mem_pool_init (&istate->ce_mem_pool, -estimate_cache_size_from_compressed (istate->cache_nr)); - }else { -mem_pool_init (&istate->ce_mem_pool, -estimate_cache_size (mmap_size, istate->cache_nr)); - } - src_offset =sizeof (*hdr);
read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2202
Quoted 10 lines
read-cache.c +6
-4-for (i =0 ; i < istate->cache_nr; i++) { -struct ondisk_cache_entry *disk_ce; -struct cache_entry *ce; -unsigned long consumed; +/* +* Locate and read the index entry offset table so that we can use it +* to multi-thread the reading of the cache entries. +*/ +if (extension_offset && nr_threads >1 ) + ieot =read_ieot_extension (mmap, mmap_size, extension_offset);
You need to free ieot at some point.
Quoted 21 lines
read-cache.c +7
-7- disk_ce = (struct ondisk_cache_entry *)(mmap + src_offset); - ce =create_from_disk (istate, disk_ce, &consumed, previous_ce); -set_index_entry (istate, i, ce); +if (ieot) + src_offset +=load_cache_entries_threaded (istate, mmap, mmap_size, src_offset, nr_threads, ieot); +else + src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); +#else + src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); +#endif - src_offset += consumed; - previous_ce = ce; - } istate->timestamp.sec = st.st_mtime; istate->timestamp.nsec =ST_MTIME_NSEC (st); --2.18.0.windows.1
Signature · 1 line
Duy
Re: [PATCH v7 7/7] read-cache: load cache entries on worker threads
Quoted 12 lines · Duy Nguyen
read-cache.c static void *load_index_extensions(void *_data) · line 1960
Quoted 6 lines · Ben Peart
read-cache.c +6
-0+/* +* A helper function that will load the specified range of cache entries +* from the memory mapped file and add them to the given index. +*/ +static unsigned long load_cache_entry_block(struct index_state *istate, + struct mem_pool *ce_mem_pool, int offset, int nr, const char *mmap,Please use unsigned long for offset (here and in the
thread_datastruct). We should useoff_tinstead, but that's out of scope. At least keep offset type consistent in here.
Unfortunately, this code is littered with different types for size and offset. "int" is the most common but there are also off_t, size_t and some unsigned long as well. Currently all of them are at least 32 bits so until we need to have an index larger than 32 bits, we should be OK. I agree, fixing them all is outside the scope of this patch.
Quoted 7 lines
Quoted 1 line
+ unsigned long
start_offset, const structcache_entry*previous_ce)I don't think you want to pass
previous_cein. You always pass NULL anyway. And if this function is about loading a block (i.e. at block boundary) then initialprevious_ce_must_ be NULL or things break horribly.
The function as written can load any arbitrary subset of cache entries as long as previous_ce is set correctly. I currently only use it on block boundaries but I don't see any good reason to limit its capabilities by moving what code passes the NULL in one function deeper.
Quoted 19 lines
read-cache.c static void *load_index_extensions(void *_data) · line 2018
Quoted 13 lines
read-cache.c +9
-0@@ -1959,20 +2007,125 @@ static void *load_index_extensions(void *_data)#define THREAD_COST (10000) +struct load_cache_entries_thread_data +{ +pthread_t pthread; +struct index_state *istate; +struct mem_pool *ce_mem_pool; +int offset; +const char *mmap; +struct index_entry_offset_table *ieot; +int ieot_offset;/* starting index into the ieot array */ If it's an index, maybe just name it
ieot_indexand we can get rid of the comment.read-cache.c static void *load_index_extensions(void *_data) · line 2019
Quoted 1 line
read-cache.c +1
-0+int ieot_work;/* count of ieot entries to process */
Quoted 8 lines
Maybe instead of saving the whole "ieot" table here. Add
struct index_entry_offset *blocks;which points to the starting block for this thread and rename that mysterious (to me)
ieot_worktonr_blocks. The thread will have access from blocks[0] to blocks[nr_blocks - 1]
Meh. Either way you have to figure out there are a block of entries and each thread is going to process some subset of those entries. You can do the base + offset math here or down in the calling function but it has to happen (and be understood) either way.
I'll rename ieot_offset to ieot_start and ieot_work to ieot_blocks which should hopefully help make it more obvious what they do.
Quoted 40 lines
Quoted 5 lines
read-cache.c +5
-0+unsigned long consumed;/* return # of bytes in index file processed */ +}; + +/* +* A thread proc to run the load_cache_entries() computation read-cache.c static void *load_index_extensions(void *_data) · line 2034
Quoted 10 lines
read-cache.c +10
-0+* across multiple background threads. +*/ +static void *load_cache_entries_thread (void *_data) +{ +struct load_cache_entries_thread_data *p = _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 );Please wrap this long line.
Quoted 9 lines
read-cache.c +9
-0+ p->offset += p->ieot->entries[i].nr; + } +return NULL ; +} + +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;read-cache.c static void *load_index_extensions(void *_data) · line 2053
Quoted 10 lines
read-cache.c +10
-0+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 (&istate->ce_mem_pool,0 ); + data =xcalloc (nr_threads,sizeof (struct load_cache_entries_thread_data ));we normally use
sizeof(*data)instead of sizeof(struct ...)
Quoted 8 lines
read-cache.c static void *load_index_extensions(void *_data) · line 2058
Quoted 5 lines
read-cache.c +5
-0+ +/* 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 ));eh.. reallocate the same "data"?
Thanks, good catch - I hate leaky code.
Quoted 34 lines
Quoted 9 lines
read-cache.c +9
-0+ + offset = ieot_offset =0 ; + ieot_work =DIV_ROUND_UP (ieot->nr, nr_threads); +for (i =0 ; i < nr_threads; i++) { +struct load_cache_entries_thread_data *p = &data[i]; +int j; + +if (ieot_offset + ieot_work > ieot->nr) + ieot_work = ieot->nr - ieot_offset;read-cache.c static void *load_index_extensions(void *_data) · line 2077
Quoted 10 lines
read-cache.c +10
-0+ + p->istate = istate; + p->offset = offset; + p->mmap = mmap; + p->ieot = ieot; + p->ieot_offset = ieot_offset; + p->ieot_work = ieot_work; + +/* create a mem_pool for each thread */ + nr =0 ;Since nr is only used in this for loop. Declare it in this scope instead of declaring it for the whole function.
read-cache.c static void *load_index_extensions(void *_data) · line 2087
Quoted 10 lines
read-cache.c +10
-0+for (j = p->ieot_offset; j < p->ieot_offset + p->ieot_work; j++) + nr += p->ieot->entries[j].nr; +if (istate->version ==4 ) { +mem_pool_init (&p->ce_mem_pool, +estimate_cache_size_from_compressed (nr)); + } +else { +mem_pool_init (&p->ce_mem_pool, +estimate_cache_size (mmap_size, nr)); + }
Quoted 5 lines
Maybe keep this
mem_pool_initcode insideload_cache_entries_thread(), similar to how you do it forload_cache_entries_thread(). It's mostly to keep this loop shorter to see (and understand), of course parallelizing thismem_pool_init()is just noise.
I understand the desire to get that part of the thread initialization out of the main line of this function (it's a bit messy between the entry counting and version differences) but I prefer to have all the thread initialization completed before creating the thread. That allows for simpler error handling and helps minimize the state you have to pass into the thread (mmap_size in this case).
Quoted 77 lines
read-cache.c static void *load_index_extensions(void *_data) · line 2095
Quoted 8 lines
read-cache.c +8
-0+ + err =pthread_create (&p->pthread,NULL , load_cache_entries_thread, p); +if (err) +die (_ ("unable to create load_cache_entries thread: %s" ),strerror (err)); + +/* increment by the number of cache entries in the ieot block being processed */ +for (j =0 ; j < ieot_work; j++) + offset += ieot->entries[ieot_offset + j].nr;I wonder if it makes things simpler if you store
cache_entry_index_ in entrie[] array instead of storing the number of entries. You can easily calculate nr then by doing entries[i].index - entries[i-1].index. And you can count multiple blocks the same way, without looping like this.Quoted 40 lines
read-cache.c +22
-3+ ieot_offset += ieot_work; + } + +for (i =0 ; i < nr_threads; i++) { +struct load_cache_entries_thread_data *p = &data[i]; + + err =pthread_join (p->pthread,NULL ); +if (err) +die (_ ("unable to join load_cache_entries thread: %s" ),strerror (err)); +mem_pool_combine (istate->ce_mem_pool, p->ce_mem_pool); + consumed += p->consumed; + } + +free (data); + +return consumed; +} +#endif +/* remember to discard_cache() before reading a different cache! */ int do_read_index (struct index_state *istate,const char *path,int must_exist) { -int fd, i; +int fd;struct stat st;unsigned long src_offset;const struct cache_header *hdr;const char *mmap;size_t mmap_size; -const struct cache_entry *previous_ce =NULL ;struct load_index_extensions p;size_t extension_offset =0 ;#ifndef NO_PTHREADS -int nr_threads; +int nr_threads, cpus; +struct index_entry_offset_table *ieot =NULL ;#endif if (istate->initialized)read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2170
Quoted 4 lines
read-cache.c +1
-0@@ -2014,10 +2167,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) p.mmap = mmap; p.mmap_size = mmap_size; + src_offset =sizeof (*hdr);OK we've been doing this since forever,
sizeof(struct cache_header)probably does not have extra padding on any supported platform.Quoted 14 lines
read-cache.c +0
-9@@ -2032,29 +2193,22 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) nr_threads--; } } -#endif - -if (istate->version ==4 ) { -mem_pool_init (&istate->ce_mem_pool, -estimate_cache_size_from_compressed (istate->cache_nr)); - }else { -mem_pool_init (&istate->ce_mem_pool, -estimate_cache_size (mmap_size, istate->cache_nr)); - }
Quoted 14 lines
read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2202
Quoted 11 lines
read-cache.c +6
-5- src_offset =sizeof (*hdr); -for (i =0 ; i < istate->cache_nr; i++) { -struct ondisk_cache_entry *disk_ce; -struct cache_entry *ce; -unsigned long consumed; +/* +* Locate and read the index entry offset table so that we can use it +* to multi-thread the reading of the cache entries. +*/ +if (extension_offset && nr_threads >1 ) + ieot =read_ieot_extension (mmap, mmap_size, extension_offset);You need to free ieot at some point.
Good catch - I hate leaky code.
Quoted 21 lines
Quoted 21 lines
read-cache.c +7
-7- disk_ce = (struct ondisk_cache_entry *)(mmap + src_offset); - ce =create_from_disk (istate, disk_ce, &consumed, previous_ce); -set_index_entry (istate, i, ce); +if (ieot) + src_offset +=load_cache_entries_threaded (istate, mmap, mmap_size, src_offset, nr_threads, ieot); +else + src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); +#else + src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); +#endif - src_offset += consumed; - previous_ce = ce; - } istate->timestamp.sec = st.st_mtime; istate->timestamp.nsec =ST_MTIME_NSEC (st); --2.18.0.windows.1
v8 cover speed up index load through parallelization
In reply to Ben PeartFrom: Ben Peart <[email protected]>
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):
read-cache.c +66 -43
@@ -1901,7 +1901,7 @@ struct index_entry_offsetstruct 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 (&extsize, 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_dataint 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 (&istate->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 = &data[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++) nr += p->ieot->entries[j].nr;if (istate->version ==4 ) {mem_pool_init (&p->ce_mem_pool,estimate_cache_size_from_compressed (nr)); - } -else { + }else {mem_pool_init (&p->ce_mem_pool,estimate_cache_size (mmap_size, nr)); }
@@ -2091,9 +2087,9 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, condie (_ ("unable to create load_cache_entries thread: %s" ),strerror (err));/* increment by the number of cache entries in the ieot block being processed */ -for (j =0 ; j < ieot_work; j++) - offset += ieot->entries[ieot_offset + j].nr; - ieot_offset += ieot_work; +for (j =0 ; j < ieot_blocks; j++) + offset += ieot->entries[ieot_start + j].nr; + ieot_start += ieot_blocks; }for (i =0 ; i < nr_threads; i++) {
@@ -2201,10 +2197,12 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)if (extension_offset && nr_threads >1 ) ieot =read_ieot_extension (mmap, mmap_size, extension_offset); -if (ieot) +if (ieot) { src_offset +=load_cache_entries_threaded (istate, mmap, mmap_size, src_offset, nr_threads, ieot); -else +free (ieot); + }else { src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); + }#else src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset);#endif
@@ -2705,9 +2703,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,struct strbuf previous_name_buf =STRBUF_INIT , *previous_name;int drop_cache_tree = istate->drop_cache_tree;off_t offset; -int ieot_work =1 ; +int ieot_entries =1 ;struct index_entry_offset_table *ieot =NULL ; -int nr; +int nr, nr_threads;for (i = removed = extended =0 ; i < entries; i++) {if (cache[i]->ce_flags &CE_REMOVE )
@@ -2742,20 +2740,24 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,return -1 ;#ifndef NO_PTHREADS -if ((nr =git_config_get_index_threads ()) !=1 ) { + nr_threads =git_config_get_index_threads (); +if (nr_threads !=1 ) {int ieot_blocks, cpus;/* * ensure default number of ieot blocks maps evenly to the -* default number of threads that will process them +* default number of threads that will process them leaving +* room for the thread to load the index extensions. */ -if (!nr) { +if (!nr_threads) { ieot_blocks = istate->cache_nr /THREAD_COST ; cpus =online_cpus ();if (ieot_blocks > cpus -1 ) ieot_blocks = cpus -1 ; }else { - ieot_blocks = nr; + ieot_blocks = nr_threads; +if (ieot_blocks > istate->cache_nr) + ieot_blocks = istate->cache_nr; }/*
@@ -2765,13 +2767,17 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,if (ieot_blocks >1 ) { ieot =xcalloc (1 ,sizeof (struct index_entry_offset_table ) + (ieot_blocks *sizeof (struct index_entry_offset ))); - ieot->nr =0 ; - ieot_work =DIV_ROUND_UP (entries, ieot_blocks); + ieot_entries =DIV_ROUND_UP (entries, ieot_blocks); } }#endif - offset =lseek (newfd,0 ,SEEK_CUR ) + write_buffer_len; + offset =lseek (newfd,0 ,SEEK_CUR ); +if (offset <0 ) { +free (ieot); +return -1 ; + } + offset += write_buffer_len; nr =0 ; previous_name = (hdr_version ==4 ) ? &previous_name_buf :NULL ;
@@ -2794,7 +2800,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, drop_cache_tree =1 ; } -if (ieot && i && (i % ieot_work ==0 )) { +if (ieot && i && (i % ieot_entries ==0 )) { ieot->entries[ieot->nr].nr = nr; ieot->entries[ieot->nr].offset = offset; ieot->nr++;
@@ -2806,7 +2812,12 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,if (previous_name) previous_name->buf[0 ] =0 ; nr =0 ; - offset =lseek (newfd,0 ,SEEK_CUR ) + write_buffer_len; + offset =lseek (newfd,0 ,SEEK_CUR ); +if (offset <0 ) { +free (ieot); +return -1 ; + } + offset += write_buffer_len; }if (ce_write_entry (&c, newfd, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) <0 ) err =-1 ;
@@ -2822,16 +2833,23 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, }strbuf_release (&previous_name_buf); -if (err) +if (err) { +free (ieot);return err; + }/* Write extension data here */ - offset =lseek (newfd,0 ,SEEK_CUR ) + write_buffer_len; + offset =lseek (newfd,0 ,SEEK_CUR ); +if (offset <0 ) { +free (ieot); +return -1 ; + } + offset += write_buffer_len; the_hash_algo->init_fn (&eoie_c);/* * Lets write out CACHE_EXT_INDEXENTRYOFFSETTABLE first so that we -* can minimze the number of extensions we have to scan through to +* can minimize the number of extensions we have to scan through to * find it during load. Write it out regardless of the * strip_extensions parameter as we need it when loading the shared * index.
@@ -2844,6 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, err =write_index_ext_header (&c, &eoie_c, newfd,CACHE_EXT_INDEXENTRYOFFSETTABLE , sb.len) <0 ||ce_write (&c, newfd, sb.buf, sb.len) <0 ;strbuf_release (&sb); +free (ieot);if (err)return -1 ; }
@@ -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);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);return NULL ; + } ieot =xmalloc (sizeof (struct index_entry_offset_table ) + (nr *sizeof (struct index_entry_offset ))); ieot->nr = nr;
### Patches
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
Nguyễn Thái Ngọc Duy (1):
read-cache.c: optimize reading index format v4
Documentation/config.txt | 7 + Documentation/technical/index-format.txt | 41 ++ config.c | 18 + config.h | 1 + read-cache.c | 774 +++++++++++++++++++---- t/README | 5 + t/t1700-split-index.sh | 13 +- 7 files changed, 739 insertions(+), 120 deletions(-)
base-commit: fe8321ec057f9231c26c29b364721568e58040f7
Signature · 1 line
2.18.0.windows.1
PATCH v8 1/7 read-cache.c: optimize reading index format v4
From: Nguyễn Thái Ngọc Duy <[email protected]>
Index format v4 requires some more computation to assemble a path based on a previous one. The current code is not very efficient because
- it doubles memory copy, we assemble the final path in a temporary first before putting it back to a cache_entry
- strbuf_remove() in expand_name_field() is not exactly a good fit for stripping a part at the end, _setlen() would do the same job and is much cheaper.
- the open-coded loop to find the end of the string in expand_name_field() can't beat an optimized strlen()
This patch avoids the temporary buffer and writes directly to the new cache_entry, which addresses the first two points. The last point could also be avoided if the total string length fits in the first 12 bits of ce_flags, if not we fall back to strlen().
Running "test-tool read-cache 100" on webkit.git (275k files), reading v2 only takes 4.226 seconds, while v4 takes 5.711 seconds, 35% more time. The patch reduces read time on v4 to 4.319 seconds.
--- read-cache.c | 128 ++++++++++++++++++++++++--------------------------- 1 file changed, 60 insertions(+), 68 deletions(-)
read-cache.c +60 -68
@@ -1713,63 +1713,24 @@ int read_index(struct index_state *istate)return read_index_from (istate,get_index_file (),get_git_dir ()); } -static struct cache_entry *cache_entry_from_ondisk (struct mem_pool *mem_pool, -struct ondisk_cache_entry *ondisk, -unsigned int flags, -const char *name, -size_t len) -{ -struct cache_entry *ce =mem_pool__ce_alloc (mem_pool, len); - - ce->ce_stat_data.sd_ctime.sec =get_be32 (&ondisk->ctime.sec); - ce->ce_stat_data.sd_mtime.sec =get_be32 (&ondisk->mtime.sec); - ce->ce_stat_data.sd_ctime.nsec =get_be32 (&ondisk->ctime.nsec); - ce->ce_stat_data.sd_mtime.nsec =get_be32 (&ondisk->mtime.nsec); - ce->ce_stat_data.sd_dev =get_be32 (&ondisk->dev); - ce->ce_stat_data.sd_ino =get_be32 (&ondisk->ino); - ce->ce_mode =get_be32 (&ondisk->mode); - ce->ce_stat_data.sd_uid =get_be32 (&ondisk->uid); - ce->ce_stat_data.sd_gid =get_be32 (&ondisk->gid); - ce->ce_stat_data.sd_size =get_be32 (&ondisk->size); - ce->ce_flags = flags & ~CE_NAMEMASK ; - ce->ce_namelen = len; - ce->index =0 ; -hashcpy (ce->oid.hash, ondisk->sha1); -memcpy (ce->name, name, len); - ce->name[len] ='\0' ; -return ce; -} - -/* -* Adjacent cache entries tend to share the leading paths, so it makes -* sense to only store the differences in later entries. In the v4 -* on-disk format of the index, each on-disk cache entry stores the -* number of bytes to be stripped from the end of the previous name, -* and the bytes to append to the result, to come up with its name. -*/ -static unsigned long expand_name_field (struct strbuf *name,const char *cp_) -{ -const unsigned char *ep, *cp = (const unsigned char *)cp_; -size_t len =decode_varint (&cp); - -if (name->len < len) -die ("malformed name field in the index" ); -strbuf_remove (name, name->len - len, len); -for (ep = cp; *ep; ep++) - ;/* find the end */ -strbuf_add (name, cp, ep - cp); -return (const char *)ep +1 - cp_; -} - -static struct cache_entry *create_from_disk (struct mem_pool *mem_pool, +static struct cache_entry *create_from_disk (struct index_state *istate,struct ondisk_cache_entry *ondisk,unsigned long *ent_size, -struct strbuf *previous_name) +const struct cache_entry *previous_ce) {struct cache_entry *ce;size_t len;const char *name;unsigned int flags; +size_t copy_len; +/* +* Adjacent cache entries tend to share the leading paths, so it makes +* sense to only store the differences in later entries. In the v4 +* on-disk format of the index, each on-disk cache entry stores the +* number of bytes to be stripped from the end of the previous name, +* and the bytes to append to the result, to come up with its name. +*/ +int expand_name_field = istate->version ==4 ;/* On-disk flags are just 16 bits */ flags =get_be16 (&ondisk->flags);
@@ -1789,21 +1750,54 @@ static struct cache_entry *create_from_disk(struct mem_pool *mem_pool,else name = ondisk->name; -if (!previous_name) { -/* v3 and earlier */ -if (len ==CE_NAMEMASK ) - len =strlen (name); - ce =cache_entry_from_ondisk (mem_pool, ondisk, flags, name, len); +if (expand_name_field) { +const unsigned char *cp = (const unsigned char *)name; +size_t strip_len, previous_len; - *ent_size =ondisk_ce_size (ce); - }else { -unsigned long consumed; - consumed =expand_name_field (previous_name, name); - ce =cache_entry_from_ondisk (mem_pool, ondisk, flags, - previous_name->buf, - previous_name->len); + previous_len = previous_ce ? previous_ce->ce_namelen :0 ; + strip_len =decode_varint (&cp); +if (previous_len < strip_len) { +if (previous_ce) +die (_ ("malformed name field in the index, near path '%s'" ), + previous_ce->name); +else +die (_ ("malformed name field in the index in the first path" )); + } + copy_len = previous_len - strip_len; + name = (const char *)cp; + } + +if (len ==CE_NAMEMASK ) { + len =strlen (name); +if (expand_name_field) + len += copy_len; + } + + ce =mem_pool__ce_alloc (istate->ce_mem_pool, len); + + ce->ce_stat_data.sd_ctime.sec =get_be32 (&ondisk->ctime.sec); + ce->ce_stat_data.sd_mtime.sec =get_be32 (&ondisk->mtime.sec); + ce->ce_stat_data.sd_ctime.nsec =get_be32 (&ondisk->ctime.nsec); + ce->ce_stat_data.sd_mtime.nsec =get_be32 (&ondisk->mtime.nsec); + ce->ce_stat_data.sd_dev =get_be32 (&ondisk->dev); + ce->ce_stat_data.sd_ino =get_be32 (&ondisk->ino); + ce->ce_mode =get_be32 (&ondisk->mode); + ce->ce_stat_data.sd_uid =get_be32 (&ondisk->uid); + ce->ce_stat_data.sd_gid =get_be32 (&ondisk->gid); + ce->ce_stat_data.sd_size =get_be32 (&ondisk->size); + ce->ce_flags = flags & ~CE_NAMEMASK ; + ce->ce_namelen = len; + ce->index =0 ; +hashcpy (ce->oid.hash, ondisk->sha1); - *ent_size = (name - ((char *)ondisk)) + consumed; +if (expand_name_field) { +if (copy_len) +memcpy (ce->name, previous_ce->name, copy_len); +memcpy (ce->name + copy_len, name, len +1 - copy_len); + *ent_size = (name - ((char *)ondisk)) + len +1 - copy_len; + }else { +memcpy (ce->name, name, len +1 ); + *ent_size =ondisk_ce_size (ce); }return ce; }
@@ -1898,7 +1892,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)struct cache_header *hdr;void *mmap;size_t mmap_size; -struct strbuf previous_name_buf =STRBUF_INIT , *previous_name; +const struct cache_entry *previous_ce =NULL ;if (istate->initialized)return istate->cache_nr;
@@ -1936,11 +1930,9 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) istate->initialized =1 ;if (istate->version ==4 ) { - previous_name = &previous_name_buf;mem_pool_init (&istate->ce_mem_pool,estimate_cache_size_from_compressed (istate->cache_nr)); }else { - previous_name =NULL ;mem_pool_init (&istate->ce_mem_pool,estimate_cache_size (mmap_size, istate->cache_nr)); }
@@ -1952,12 +1944,12 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)unsigned long consumed; disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset); - ce =create_from_disk (istate->ce_mem_pool, disk_ce, &consumed, previous_name); + ce =create_from_disk (istate, disk_ce, &consumed, previous_ce);set_index_entry (istate, i, ce); src_offset += consumed; + previous_ce = ce; } -strbuf_release (&previous_name_buf); istate->timestamp.sec = st.st_mtime; istate->timestamp.nsec =ST_MTIME_NSEC (st);
Signature · 1 line
2.18.0.windows.1
PATCH v8 2/7 read-cache: clean up casting and byte decoding
In reply to Ben PeartFrom: Ben Peart <[email protected]>
This patch does a clean up pass to minimize the casting required to work with the memory mapped index (mmap).
It also makes the decoding of network byte order more consistent by using get_be32() where possible.
--- read-cache.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
read-cache.c +11 -12
@@ -1650,7 +1650,7 @@ int verify_index_checksum;/* Allow fsck to force verification of the cache entry order. */ int verify_ce_order; -static int verify_hdr (struct cache_header *hdr,unsigned long size) +static int verify_hdr (const struct cache_header *hdr,unsigned long size) {git_hash_ctx c;unsigned char hash[GIT_MAX_RAWSZ ];
@@ -1674,7 +1674,7 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size) }static int read_index_extension (struct index_state *istate, -const char *ext,void *data,unsigned long sz) +const char *ext,const char *data,unsigned long sz) {switch (CACHE_EXT (ext)) {case CACHE_EXT_TREE :
@@ -1889,8 +1889,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)int fd, i;struct stat st;unsigned long src_offset; -struct cache_header *hdr; -void *mmap; +const struct cache_header *hdr; +const char *mmap;size_t mmap_size;const struct cache_entry *previous_ce =NULL ;
@@ -1918,7 +1918,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)die_errno ("unable to map index file" );close (fd); - hdr = mmap; + hdr = (const struct cache_header *)mmap;if (verify_hdr (hdr, mmap_size) <0 ) goto unmap;
@@ -1943,7 +1943,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)struct cache_entry *ce;unsigned long consumed; - disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset); + disk_ce = (struct ondisk_cache_entry *)(mmap + src_offset); ce =create_from_disk (istate, disk_ce, &consumed, previous_ce);set_index_entry (istate, i, ce);
@@ -1961,21 +1961,20 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)* in 4-byte network byte order. */ uint32_t extsize; -memcpy (&extsize, (char *)mmap + src_offset +4 ,4 ); - extsize =ntohl (extsize); + extsize =get_be32 (mmap + src_offset +4 );if (read_index_extension (istate, - (const char *) mmap + src_offset, - (char *) mmap + src_offset +8 , + mmap + src_offset, + mmap + src_offset +8 , extsize) <0 ) goto unmap; src_offset +=8 ; src_offset += extsize; } -munmap (mmap, mmap_size); +munmap ((void *)mmap, mmap_size);return istate->cache_nr; unmap: -munmap (mmap, mmap_size); +munmap ((void *)mmap, mmap_size);die ("index file corrupt" ); }
Signature · 1 line
2.18.0.windows.1
PATCH v8 3/7 eoie: add End of Index Entry (EOIE) extension
In reply to Ben PeartFrom: Ben Peart <[email protected]>
The End of Index Entry (EOIE) is used to locate the end of the variable length index entries and the beginning of the extensions. Code can take advantage of this to quickly locate the index extensions without having to parse through all of the index entries.
The EOIE extension is always written out to the index file including to the shared index when using the split index feature. Because it is always written out, the SHA checksums in t/t1700-split-index.sh were updated to reflect its inclusion.
It is written as an optional extension to ensure compatibility with other git implementations that do not yet support it. It is always written out to ensure it is available as often as possible to speed up index operations.
Because it must be able to be loaded before the variable length cache entries and other index extensions, this extension must be written last. The signature for this extension is { 'E', 'O', 'I', 'E' }.
The extension consists of:
- 32-bit offset to the end of the index entries
- 160-bit SHA-1 over the extension types and their sizes (but not their contents). E.g. if we have "TREE" extension that is N-bytes long, "REUC" extension that is M-bytes long, followed by "EOIE", then the hash would be:
SHA-1("TREE" + <binary representation of N> +
"REUC" + <binary representation of M>)
--- Documentation/technical/index-format.txt | 23 ++++ read-cache.c | 158 +++++++++++++++++++++-- t/t1700-split-index.sh | 8 +- 3 files changed, 177 insertions(+), 12 deletions(-)
Documentation/technical/index-format.txt +23 -0
@@ -314,3 +314,26 @@ The remaining data of each directory block is grouped by type: - An ewah bitmap, the n-th bit indicates whether the n-th index entry is not CE_FSMONITOR_VALID. + +== End of Index Entry + + The End of Index Entry (EOIE) is used to locate the end of the variable + length index entries and the begining of the extensions. Code can take + advantage of this to quickly locate the index extensions without having + to parse through all of the index entries. + + Because it must be able to be loaded before the variable length cache + entries and other index extensions, this extension must be written last. + The signature for this extension is { 'E', 'O', 'I', 'E' }. + + The extension consists of: + + - 32-bit offset to the end of the index entries + + - 160-bit SHA-1 over the extension types and their sizes (but not + their contents). E.g. if we have "TREE" extension that is N-bytes + long, "REUC" extension that is M-bytes long, followed by "EOIE", + then the hash would be: + + SHA-1("TREE" + <binary representation of N> + + "REUC" + <binary representation of M>)
read-cache.c +150 -8
240 lines not on this page · raw message
t/t1700-split-index.sh +4 -4
@@ -15,11 +15,11 @@ test_expect_success 'enable split index' ' indexversion=$(test-tool index-version <.git/index) && if test "$indexversion" = "4" then - own=432ef4b63f32193984f339431fd50ca796493569 - base=508851a7f0dfa8691e9f69c7f055865389012491 + own=3527df833c6c100d3d1d921a9a782d62a8be4b58 + base=746f7ab2ed44fb839efdfbffcf399d0b113fb4cb else - own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339 - base=39d890139ee5356c7ef572216cebcd27aa41f9df + own=5e9b60117ece18da410ddecc8b8d43766a0e4204 + base=4370042739b31cd17a5c5cd6043a77c9a00df113 fi && cat >expect <<-EOF && own $own
Signature · 1 line
2.18.0.windows.1
PATCH v8 4/7 config: add new index.threads config setting
In reply to Ben PeartFrom: Ben Peart <[email protected]>
Add support for a new index.threads config setting which will be used to control the threading code in do_read_index(). A value of 0 will tell the index code to automatically determine the correct number of threads to use. A value of 1 will make the code single threaded. A value greater than 1 will set the maximum number of threads to use.
For testing purposes, this setting can be overwritten by setting the GIT_TEST_INDEX_THREADS=<n> environment variable to a value greater than 0.
--- Documentation/config.txt | 7 +++++++ config.c | 18 ++++++++++++++++++ config.h | 1 + t/README | 5 +++++ t/t1700-split-index.sh | 5 +++++ 5 files changed, 36 insertions(+)
Documentation/config.txt +7 -0
@@ -2413,6 +2413,13 @@ imap:: The configuration variables in the 'imap' section are described in linkgit:git-imap-send[1]. +index.threads:: + Specifies the number of threads to spawn when loading the index. + This is meant to reduce index load time on multiprocessor machines. + Specifying 0 or 'true' will cause Git to auto-detect the number of + CPU's and set the number of threads accordingly. Specifying 1 or + 'false' will disable multithreading. Defaults to 'true'. + index.version:: Specify the version with which new index files should be initialized. This does not affect existing repositories.
config.c +18 -0
@@ -2289,6 +2289,24 @@ int git_config_get_fsmonitor(void)return 0 ; } +int git_config_get_index_threads (void ) +{ +int is_bool, val =0 ; + + val =git_env_ulong ("GIT_TEST_INDEX_THREADS" ,0 ); +if (val) +return val; + +if (!git_config_get_bool_or_int ("index.threads" , &is_bool, &val)) { +if (is_bool) +return val ?0 :1 ; +else +return val; + } + +return 0 ;/* auto */ +} +NORETURN void git_die_config_linenr(const char *key, const char *filename, int linenr) {
config.h +1 -0
@@ -250,6 +250,7 @@ extern int git_config_get_untracked_cache(void);extern int git_config_get_split_index (void );extern int git_config_get_max_percent_split_change (void );extern int git_config_get_fsmonitor (void ); +extern int git_config_get_index_threads (void );/* This dies if the configured or default date is in the future */ extern int git_config_get_expiry (const char *key,const char **output);
t/README +5 -0
@@ -327,6 +327,11 @@ GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to be written after every 'git commit' command, and overrides the 'core.commitGraph' setting to true. +GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading +of the index for the whole test suite by bypassing the default number of +cache entries and thread minimums. Setting this to 1 will make the +index loading single threaded. + Naming Tests ------------
t/t1700-split-index.sh +5 -0
@@ -6,7 +6,12 @@ test_description='split index mode tests' # We need total control of index splitting here sane_unset GIT_TEST_SPLIT_INDEX + +# Testing a hard coded SHA against an index with an extension +# that can vary from run to run is problematic so we disable +# those extensions. sane_unset GIT_FSMONITOR_TEST +sane_unset GIT_TEST_INDEX_THREADS test_expect_success 'enable split index' ' git config splitIndex.maxPercentChange 100 &&
Signature · 1 line
2.18.0.windows.1
PATCH v8 5/7 read-cache: load cache extensions on a worker thread
In reply to Ben PeartFrom: Ben Peart <[email protected]>
This patch helps address the CPU cost of loading the index by loading the cache extensions on a worker thread in parallel with loading the cache entries.
In some cases, loading the extensions takes longer than loading the cache entries so this patch utilizes the new EOIE to start the thread to load the extensions before loading all the cache entries in parallel.
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.
I used p0002-read-cache.sh to generate some performance data:
Test w/100,000 files reduced the time by 0.53% Test w/1,000,000 files reduced the time by 27.78%
--- read-cache.c | 95 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 16 deletions(-)
read-cache.c +79 -16
@@ -23,6 +23,7 @@#include "split-index.h" #include "utf8.h" #include "fsmonitor.h" +#include "thread-utils.h" /* Mask for the name length in ce_flags in the on-disk index */
@@ -1890,6 +1891,44 @@ static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)static size_t read_eoie_extension (const char *mmap,size_t mmap_size);static void write_eoie_extension (struct strbuf *sb,git_hash_ctx *eoie_context,size_t offset); +struct load_index_extensions +{ +#ifndef NO_PTHREADS +pthread_t pthread; +#endif +struct index_state *istate; +const char *mmap; +size_t mmap_size; +unsigned long src_offset; +}; + +static void *load_index_extensions (void *_data) +{ +struct load_index_extensions *p = _data; +unsigned long src_offset = p->src_offset; + +while (src_offset <= p->mmap_size - the_hash_algo->rawsz -8 ) { +/* After an array of active_nr index entries, +* there can be arbitrary number of extended +* sections, each of which is prefixed with +* extension name (4-byte) and section length +* in 4-byte network byte order. +*/ +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 , + extsize) <0 ) { +munmap ((void *)p->mmap, p->mmap_size); +die (_ ("index file corrupt" )); + } + src_offset +=8 ; + src_offset += extsize; + } + +return NULL ; +} +/* remember to discard_cache() before reading a different cache! */ int do_read_index (struct index_state *istate,const char *path,int must_exist) {
@@ -1900,6 +1939,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)const char *mmap;size_t mmap_size;const struct cache_entry *previous_ce =NULL ; +struct load_index_extensions p; +size_t extension_offset =0 ; +#ifndef NO_PTHREADS +int nr_threads; +#endif if (istate->initialized)return istate->cache_nr;
@@ -1936,6 +1980,30 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) istate->cache =xcalloc (istate->cache_alloc,sizeof (*istate->cache)); istate->initialized =1 ; + p.istate = istate; + p.mmap = mmap; + p.mmap_size = mmap_size; + +#ifndef NO_PTHREADS + nr_threads =git_config_get_index_threads (); +if (!nr_threads) + nr_threads =online_cpus (); + +if (nr_threads >1 ) { + extension_offset =read_eoie_extension (mmap, mmap_size); +if (extension_offset) { +int err; + + p.src_offset = extension_offset; + err =pthread_create (&p.pthread,NULL , load_index_extensions, &p); +if (err) +die (_ ("unable to create load_index_extensions thread: %s" ),strerror (err)); + + nr_threads--; + } + } +#endif +if (istate->version ==4 ) {mem_pool_init (&istate->ce_mem_pool,estimate_cache_size_from_compressed (istate->cache_nr));
@@ -1960,22 +2028,17 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) istate->timestamp.sec = st.st_mtime; istate->timestamp.nsec =ST_MTIME_NSEC (st); -while (src_offset <= mmap_size - the_hash_algo->rawsz -8 ) { -/* After an array of active_nr index entries, -* there can be arbitrary number of extended -* sections, each of which is prefixed with -* extension name (4-byte) and section length -* in 4-byte network byte order. -*/ -uint32_t extsize; - extsize =get_be32 (mmap + src_offset +4 ); -if (read_index_extension (istate, - mmap + src_offset, - mmap + src_offset +8 , - extsize) <0 ) - goto unmap; - src_offset +=8 ; - src_offset += extsize; +/* if we created a thread, join it otherwise load the extensions on the primary thread */ +#ifndef NO_PTHREADS +if (extension_offset) { +int ret =pthread_join (p.pthread,NULL ); +if (ret) +die (_ ("unable to join load_index_extensions thread: %s" ),strerror (ret)); + } +#endif +if (!extension_offset) { + p.src_offset = src_offset; +load_index_extensions (&p); }munmap ((void *)mmap, mmap_size);return istate->cache_nr;
Signature · 1 line
2.18.0.windows.1
PATCH v8 6/7 ieot: add Index Entry Offset Table (IEOT) extension
In reply to Ben PeartFrom: Ben Peart <[email protected]>
This patch enables addressing the CPU cost of loading the index by adding additional data to the index that will allow us to efficiently multi- thread the loading and conversion of cache entries.
It accomplishes this by adding an (optional) index extension that is a table of offsets to blocks of cache entries in the index file. To make this work for V4 indexes, when writing the cache entries, it periodically "resets" the prefix-compression by encoding the current entry as if the path name for the previous entry is completely different and saves the offset of that entry in the IEOT. Basically, with V4 indexes, it generates offsets into blocks of prefix-compressed entries.
--- Documentation/technical/index-format.txt | 18 +++ read-cache.c | 196 ++++++++++++++++++++++- 2 files changed, 211 insertions(+), 3 deletions(-)
Documentation/technical/index-format.txt +18 -0
@@ -337,3 +337,21 @@ The remaining data of each directory block is grouped by type: SHA-1("TREE" + <binary representation of N> + "REUC" + <binary representation of M>) + +== Index Entry Offset Table + + The Index Entry Offset Table (IEOT) is used to help address the CPU + cost of loading the index by enabling multi-threading the process of + converting cache entries from the on-disk format to the in-memory format. + The signature for this extension is { 'I', 'E', 'O', 'T' }. + + The extension consists of: + + - 32-bit version (currently 1) + + - A number of index offset entries each consisting of: + + - 32-bit offset from the begining of the file to the first cache entry + in this block of entries. + + - 32-bit count of cache entries in this block
read-cache.c +193 -3
260 lines not on this page · raw message
Signature · 1 line
2.18.0.windows.1
PATCH v8 7/7 read-cache: load cache entries on worker threads
In reply to Ben PeartFrom: Ben Peart <[email protected]>
This patch helps address the CPU cost of loading the index by utilizing the Index Entry Offset Table (IEOT) to divide loading and conversion of the cache entries across multiple threads in parallel.
I used p0002-read-cache.sh to generate some performance data:
Test w/100,000 files reduced the time by 32.24%
Test w/1,000,000 files reduced the time by -4.77%
Note that on the 1,000,000 files case, multi-threading the cache entry parsing does not yield a performance win. This is because the cost to parse the index extensions in this repo, far outweigh the cost of loading the cache entries.
The high cost of parsing the index extensions is driven by the cache tree and the untracked cache extensions. As this is currently the longest pole, any reduction in this time will reduce the overall index load times so is worth further investigation in another patch series.
--- read-cache.c | 230 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 193 insertions(+), 37 deletions(-)
read-cache.c +193 -37
@@ -1720,7 +1720,8 @@ int read_index(struct index_state *istate)return read_index_from (istate,get_index_file (),get_git_dir ()); } -static struct cache_entry *create_from_disk (struct index_state *istate, +static struct cache_entry *create_from_disk (struct mem_pool *ce_mem_pool, +unsigned int version,struct ondisk_cache_entry *ondisk,unsigned long *ent_size,const struct cache_entry *previous_ce)
@@ -1737,7 +1738,7 @@ static struct cache_entry *create_from_disk(struct index_state *istate,* number of bytes to be stripped from the end of the previous name, * and the bytes to append to the result, to come up with its name. */ -int expand_name_field = istate->version ==4 ; +int expand_name_field = version ==4 ;/* On-disk flags are just 16 bits */ flags =get_be16 (&ondisk->flags);
@@ -1761,16 +1762,17 @@ static struct cache_entry *create_from_disk(struct index_state *istate,const unsigned char *cp = (const unsigned char *)name;size_t strip_len, previous_len; - previous_len = previous_ce ? previous_ce->ce_namelen :0 ; +/* If we're at the begining of a block, ignore the previous name */ strip_len =decode_varint (&cp); -if (previous_len < strip_len) { -if (previous_ce) +if (previous_ce) { + previous_len = previous_ce->ce_namelen; +if (previous_len < strip_len)die (_ ("malformed name field in the index, near path '%s'" ), - previous_ce->name); -else -die (_ ("malformed name field in the index in the first path" )); + previous_ce->name); + copy_len = previous_len - strip_len; + }else { + copy_len =0 ; } - copy_len = previous_len - strip_len; name = (const char *)cp; }
@@ -1780,7 +1782,7 @@ static struct cache_entry *create_from_disk(struct index_state *istate, len += copy_len; } - ce =mem_pool__ce_alloc (istate->ce_mem_pool, len); + ce =mem_pool__ce_alloc (ce_mem_pool, len); ce->ce_stat_data.sd_ctime.sec =get_be32 (&ondisk->ctime.sec); ce->ce_stat_data.sd_mtime.sec =get_be32 (&ondisk->mtime.sec);
@@ -1948,6 +1950,52 @@ static void *load_index_extensions(void *_data)return NULL ; } +/* +* A helper function that will load the specified range of cache entries +* from the memory mapped file and add them to the given index. +*/ +static unsigned long load_cache_entry_block (struct index_state *istate, +struct mem_pool *ce_mem_pool,int offset,int nr,const char *mmap, +unsigned long start_offset,const struct cache_entry *previous_ce) +{ +int i; +unsigned long src_offset = start_offset; + +for (i = offset; i < offset + nr; i++) { +struct ondisk_cache_entry *disk_ce; +struct cache_entry *ce; +unsigned long consumed; + + disk_ce = (struct ondisk_cache_entry *)(mmap + src_offset); + ce =create_from_disk (ce_mem_pool, istate->version, disk_ce, &consumed, previous_ce); +set_index_entry (istate, i, ce); + + src_offset += consumed; + previous_ce = ce; + } +return src_offset - start_offset; +} + +static unsigned long load_all_cache_entries (struct index_state *istate, +const char *mmap,size_t mmap_size,unsigned long src_offset) +{ +unsigned long consumed; + +if (istate->version ==4 ) { +mem_pool_init (&istate->ce_mem_pool, +estimate_cache_size_from_compressed (istate->cache_nr)); + }else { +mem_pool_init (&istate->ce_mem_pool, +estimate_cache_size (mmap_size, istate->cache_nr)); + } + + consumed =load_cache_entry_block (istate, istate->ce_mem_pool, +0 , istate->cache_nr, mmap, src_offset,NULL ); +return consumed; +} + +#ifndef NO_PTHREADS +/* * Mostly randomly chosen maximum thread counts: we * cap the parallelism to online_cpus() threads, and we want
@@ -1957,20 +2005,123 @@ static void *load_index_extensions(void *_data)#define THREAD_COST (10000) +struct load_cache_entries_thread_data +{ +pthread_t pthread; +struct index_state *istate; +struct mem_pool *ce_mem_pool; +int offset; +const char *mmap; +struct index_entry_offset_table *ieot; +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 */ +}; + +/* +* A thread proc to run the load_cache_entries() computation +* across multiple background threads. +*/ +static void *load_cache_entries_thread (void *_data) +{ +struct load_cache_entries_thread_data *p = _data; +int i; + +/* iterate across all ieot blocks assigned to this thread */ +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 ; +} + +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_blocks, ieot_start, err; +struct load_cache_entries_thread_data *data; +unsigned long consumed =0 ; + +/* a little sanity checking */ +if (istate->name_hash_initialized) +BUG ("the name hash isn't thread safe" ); + +mem_pool_init (&istate->ce_mem_pool,0 ); + +/* 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 (*data)); + + 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 = &data[i]; +int nr, j; + +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_start = ieot_start; + p->ieot_blocks = ieot_blocks; + +/* create a mem_pool for each thread */ + nr =0 ; +for (j = p->ieot_start; j < p->ieot_start + p->ieot_blocks; j++) + nr += p->ieot->entries[j].nr; +if (istate->version ==4 ) { +mem_pool_init (&p->ce_mem_pool, +estimate_cache_size_from_compressed (nr)); + }else { +mem_pool_init (&p->ce_mem_pool, +estimate_cache_size (mmap_size, nr)); + } + + err =pthread_create (&p->pthread,NULL , load_cache_entries_thread, p); +if (err) +die (_ ("unable to create load_cache_entries thread: %s" ),strerror (err)); + +/* increment by the number of cache entries in the ieot block being processed */ +for (j =0 ; j < ieot_blocks; j++) + offset += ieot->entries[ieot_start + j].nr; + ieot_start += ieot_blocks; + } + +for (i =0 ; i < nr_threads; i++) { +struct load_cache_entries_thread_data *p = &data[i]; + + err =pthread_join (p->pthread,NULL ); +if (err) +die (_ ("unable to join load_cache_entries thread: %s" ),strerror (err)); +mem_pool_combine (istate->ce_mem_pool, p->ce_mem_pool); + consumed += p->consumed; + } + +free (data); + +return consumed; +} +#endif +/* remember to discard_cache() before reading a different cache! */ int do_read_index (struct index_state *istate,const char *path,int must_exist) { -int fd, i; +int fd;struct stat st;unsigned long src_offset;const struct cache_header *hdr;const char *mmap;size_t mmap_size; -const struct cache_entry *previous_ce =NULL ;struct load_index_extensions p;size_t extension_offset =0 ;#ifndef NO_PTHREADS -int nr_threads; +int nr_threads, cpus; +struct index_entry_offset_table *ieot =NULL ;#endif if (istate->initialized)
@@ -2012,10 +2163,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) p.mmap = mmap; p.mmap_size = mmap_size; + src_offset =sizeof (*hdr); +#ifndef NO_PTHREADS nr_threads =git_config_get_index_threads (); -if (!nr_threads) - nr_threads =online_cpus (); + +/* TODO: does creating more threads than cores help? */ +if (!nr_threads) { + nr_threads = istate->cache_nr /THREAD_COST ; + cpus =online_cpus (); +if (nr_threads > cpus) + nr_threads = cpus; + }if (nr_threads >1 ) { extension_offset =read_eoie_extension (mmap, mmap_size);
@@ -2030,29 +2189,24 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) nr_threads--; } } -#endif -if (istate->version ==4 ) { -mem_pool_init (&istate->ce_mem_pool, -estimate_cache_size_from_compressed (istate->cache_nr)); +/* +* Locate and read the index entry offset table so that we can use it +* to multi-thread the reading of the cache entries. +*/ +if (extension_offset && nr_threads >1 ) + ieot =read_ieot_extension (mmap, mmap_size, extension_offset); + +if (ieot) { + src_offset +=load_cache_entries_threaded (istate, mmap, mmap_size, src_offset, nr_threads, ieot); +free (ieot); }else { -mem_pool_init (&istate->ce_mem_pool, -estimate_cache_size (mmap_size, istate->cache_nr)); + src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); } +#else + src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); +#endif - src_offset =sizeof (*hdr); -for (i =0 ; i < istate->cache_nr; i++) { -struct ondisk_cache_entry *disk_ce; -struct cache_entry *ce; -unsigned long consumed; - - disk_ce = (struct ondisk_cache_entry *)(mmap + src_offset); - ce =create_from_disk (istate, disk_ce, &consumed, previous_ce); -set_index_entry (istate, i, ce); - - src_offset += consumed; - previous_ce = ce; - } istate->timestamp.sec = st.st_mtime; istate->timestamp.nsec =ST_MTIME_NSEC (st);
@@ -2549,7 +2703,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,struct strbuf previous_name_buf =STRBUF_INIT , *previous_name;int drop_cache_tree = istate->drop_cache_tree;off_t offset; -int ieot_blocks =1 ; +int ieot_entries =1 ;struct index_entry_offset_table *ieot =NULL ;int nr, nr_threads;
@@ -2602,6 +2756,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, ieot_blocks = cpus -1 ; }else { ieot_blocks = nr_threads; +if (ieot_blocks > istate->cache_nr) + ieot_blocks = istate->cache_nr; }/*
@@ -2611,7 +2767,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,if (ieot_blocks >1 ) { ieot =xcalloc (1 ,sizeof (struct index_entry_offset_table ) + (ieot_blocks *sizeof (struct index_entry_offset ))); - ieot_blocks =DIV_ROUND_UP (entries, ieot_blocks); + ieot_entries =DIV_ROUND_UP (entries, ieot_blocks); } }#endif
@@ -2644,7 +2800,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, drop_cache_tree =1 ; } -if (ieot && i && (i % ieot_blocks ==0 )) { +if (ieot && i && (i % ieot_entries ==0 )) { ieot->entries[ieot->nr].nr = nr; ieot->entries[ieot->nr].offset = offset; ieot->nr++;
Signature · 1 line
2.18.0.windows.1
Re: [PATCH v8 7/7] read-cache: load cache entries on worker threads
read-cache.c static void *load_index_extensions(void *_data) · line 2040
Quoted 2 lines · Ben Peart
read-cache.c +2
-0+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:
read-cache.c static void *load_index_extensions(void *_data) · line 2059
Quoted 4 lines
read-cache.c +3
-0+ 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?
-Peff
Re: [PATCH v8 7/7] read-cache: load cache entries on worker threads
Quoted 12 lines · Jeff King
read-cache.c static void *load_index_extensions(void *_data) · line 2040
Quoted 2 lines · Ben Peart
read-cache.c +2
-0+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_offsetparameter isn't used in this function.In early versions of the series, it was used to feed the
p->start_offsetfield of eachload_cache_entries_thread_data. But after the switch to ieot, we don't, and instead feedp->ieot_start. But we always begin that at 0.
Quoted 8 lines
Is that right (and we can drop the parameter), or should this logic:
read-cache.c static void *load_index_extensions(void *_data) · line 2059
Quoted 4 lines
read-cache.c +3
-0+ offset = ieot_start =0 ; + ieot_blocks =DIV_ROUND_UP (ieot->nr, nr_threads); +for (i =0 ; i < nr_threads; i++) {be starting at
src_offsetinstead 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).
Re: [PATCH v8 7/7] read-cache: load cache entries on worker threads
Quoted 37 lines · Junio C Hamano
Quoted 12 lines · Jeff King
read-cache.c static void *load_index_extensions(void *_data) · line 2040
Quoted 2 lines · Ben Peart
read-cache.c +2
-0+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_offsetparameter isn't used in this function.In early versions of the series, it was used to feed the
p->start_offsetfield of eachload_cache_entries_thread_data. But after the switch to ieot, we don't, and instead feedp->ieot_start. But we always begin that at 0.Quoted 8 lines
Is that right (and we can drop the parameter), or should this logic:
read-cache.c static void *load_index_extensions(void *_data) · line 2059
Quoted 4 lines
read-cache.c +3
-0+ offset = ieot_start =0 ; + ieot_blocks =DIV_ROUND_UP (ieot->nr, nr_threads); +for (i =0 ; i < nr_threads; i++) {be starting at
src_offsetinstead 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 sameif()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 thesizeof(*hdr)that initializessrc_offsetis not used by the codepath.
Quoted 9 lines
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_offsetfor 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 existsextension_offsetis not 0, so we do not make the finalload_index_extensions()call in the caller that usessrc_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.
Re: [PATCH v8 0/7] speed up index load through parallelization
In reply to Ben PeartQuoted 9 lines · Ben Peart
From: Ben Peart <[email protected]>
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_offsettoieot_startandieot_worktoieot_blocksin 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.
Re: [PATCH v8 0/7] speed up index load through parallelization
In reply to Ben Peartread-cache.c static struct index_entry_offset_table *read_ieot_extension(const char *mmap, si · line 3483
Quoted 7 lines · Ben Peart
read-cache.c +2
-1@@ -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.
read-cache.c static struct index_entry_offset_table *read_ieot_extension(const char *mmap, si · line 3491
Quoted 9 lines
read-cache.c +3
-1return 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.
Signature · 1 line
Duy
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.
--- read-cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
read-cache.c +1 -1
@@ -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)
Signature · 1 line
2.18.0.windows.1
Quoted 25 lines · Duy Nguyen
Review static struct index_entry_offset_table *read_ieot_extension(const char *mmap, si · line 3483
Quoted 7 lines · Ben Peart
(quoted diff) +2
-1@@ -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.Review static struct index_entry_offset_table *read_ieot_extension(const char *mmap, si · line 3491
Quoted 9 lines
(quoted diff) +3
-1return 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.
cover Avoid confusing messages from new index extensions (Re: [PATCH v8 0/7] speed up index load through parallelization)
In reply to Ben PeartHi,
Quoted 7 lines · Ben Peart
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(-)
PATCH 1/3 eoie: default to not writing EOIE section
Since 3b1d9e04 (eoie: add End of Index Entry (EOIE) extension, 2018-10-10) Git defaults to writing the new EOIE section when writing out an index file. Usually that is a good thing because it improves threaded performance, but when a Git repository is shared with older versions of Git, it produces a confusing warning:
$ git status ignoring EOIE extension HEAD detached at 371ed0defa nothing to commit, working tree clean
Let's introduce the new index extension more gently. First we'll roll out the new version of Git that understands it, and then once sufficiently many users are using such a version, we can flip the default to writing it by default.
Introduce a '[index] recordEndOfIndexEntries' configuration variable to allow interested users to benefit from this index extension early.
--- Documentation/config.txt | 7 +++++++ read-cache.c | 11 ++++++++++- t/t1700-split-index.sh | 11 +++++++---- 3 files changed, 24 insertions(+), 5 deletions(-)
Documentation/config.txt +7 -0
@@ -2188,6 +2188,13 @@ imap:: The configuration variables in the 'imap' section are described in linkgit:git-imap-send[1]. +index.recordEndOfIndexEntries:: + Specifies whether the index file should include an "End Of Index + Entry" section. This reduces index load time on multiprocessor + machines but produces a message "ignoring EOIE extension" when + reading the index using Git versions before 2.20. Defaults to + 'false'. + index.threads:: Specifies the number of threads to spawn when loading the index. This is meant to reduce index load time on multiprocessor machines.
read-cache.c +10 -1
@@ -2698,6 +2698,15 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfilerollback_lock_file (lockfile); } +static int record_eoie (void ) +{ +int val; + +if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ; +} +/* * On success, `tempfile` is closed. If it is the temporary file * of a `struct lock_file`, we will therefore effectively perform
@@ -2945,7 +2954,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,* read. Write it out regardless of the strip_extensions parameter as we need it * when loading the shared index. */ -if (offset) { +if (offset &&record_eoie ()) {struct strbuf sb =STRBUF_INIT ;write_eoie_extension (&sb, &eoie_c, offset);
t/t1700-split-index.sh +7 -4
@@ -25,14 +25,17 @@ test_expect_success 'enable split index' ' git update-index --split-index && test-tool dump-split-index .git/index >actual && indexversion=$(test-tool index-version <.git/index) && + + # NEEDSWORK: Stop hard-coding checksums. if test "$indexversion" = "4" then - own=3527df833c6c100d3d1d921a9a782d62a8be4b58 - base=746f7ab2ed44fb839efdfbffcf399d0b113fb4cb + own=432ef4b63f32193984f339431fd50ca796493569 + base=508851a7f0dfa8691e9f69c7f055865389012491 else - own=5e9b60117ece18da410ddecc8b8d43766a0e4204 - base=4370042739b31cd17a5c5cd6043a77c9a00df113 + own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339 + base=39d890139ee5356c7ef572216cebcd27aa41f9df fi && + cat >expect <<-EOF && own $own base $base
Signature · 1 line
2.19.1.930.g4563a0d9d0
Re: [PATCH 1/3] eoie: default to not writing EOIE section
Quoted 11 lines · Jonathan Nieder
Since
3b1d9e04(eoie: add End of Index Entry (EOIE) extension, 2018-10-10) Git defaults to writing the new EOIE section when writing out an index file. Usually that is a good thing because it improves threaded performance, but when a Git repository is shared with older versions of Git, it produces a confusing warning:$ git status ignoring EOIE extension HEAD detached at 371ed0defa nothing to commit, working tree clean
Quoted 7 lines
Let's introduce the new index extension more gently. First we'll roll out the new version of Git that understands it, and then once sufficiently many users are using such a version, we can flip the default to writing it by default.
Introduce a '[index] recordEndOfIndexEntries' configuration variable to allow interested users to benefit from this index extension early.
Thanks. I am in principle OK with this approach. In fact, I suspect that the default may want to be dynamically determined, and we give this knob to let the users further force their preference. When no extension that benefits from multi-threading is written, the default can stay "no" in future versions of Git, for example.
Documentation/config.txt
Quoted 2 lines
Documentation/config.txt +0
-0
The timing is a bit unfortunate for any topic to touch this file, and contrib/cocci would not help us in this case X-<.
Quoted 6 lines
read-cache.c +0
-0@@ -2698,6 +2698,15 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfilerollback_lock_file (lockfile);
read-cache.c void update_index_if_able(struct index_state *istate, struct lock_file *lockfile · line 2708
Quoted 10 lines
read-cache.c +8
-0} +static int record_eoie (void ) +{ +int val; + +if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ; +}
Unconditionally defaulting to no in this round is perfectly fine. Let's make a mental note that this is the place to decide dynamic default in the future when we want to. It would probably have to ask around various "extension writing" helpers if they want to have a say in the outcome (e.g. if there are very many cache entries in the istate, the entry offset table may want to be written and otherwise not).
Quoted 14 lines
read-cache.c +1
-1@@ -2945,7 +2954,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,* read. Write it out regardless of the strip_extensions parameter as we need it * when loading the shared index. */ -if (offset) { +if (offset &&record_eoie ()) {struct strbuf sb =STRBUF_INIT ;write_eoie_extension (&sb, &eoie_c, offset);t/t1700-split-index.sh +0
-0
t/t1700-split-index.sh test_expect_success 'enable split index' ' · line 29
Quoted 5 lines
t/t1700-split-index.sh +2
-0@@ -25,14 +25,17 @@ test_expect_success 'enable split index' ' git update-index --split-index && test-tool dump-split-index .git/index >actual && indexversion=$(test-tool index-version <.git/index) && + + # NEEDSWORK: Stop hard-coding checksums.
Also let's stop hard-coding the assumption that the new knob is off by default. Ideally, you'd want to test both cases, right?
Perhaps you'd call "git update-index --split-index" we see in the precontext twice, with "-c VAR=false" and "-c VAR=true", to prepare "actual.without-eoie" and "actual.with-eoie", or something like that?
Thanks.
Quoted 16 lines
t/t1700-split-index.sh +5
-4if test "$indexversion" = "4" then - own=3527df833c6c100d3d1d921a9a782d62a8be4b58 - base=746f7ab2ed44fb839efdfbffcf399d0b113fb4cb + own=432ef4b63f32193984f339431fd50ca796493569 + base=508851a7f0dfa8691e9f69c7f055865389012491 else - own=5e9b60117ece18da410ddecc8b8d43766a0e4204 - base=4370042739b31cd17a5c5cd6043a77c9a00df113 + own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339 + base=39d890139ee5356c7ef572216cebcd27aa41f9df fi && + cat >expect <<-EOF && own $own base $base
Re: [PATCH 1/3] eoie: default to not writing EOIE section
Quoted 18 lines · Junio C Hamano
Quoted 16 lines · Jonathan Nieder
Since
3b1d9e04(eoie: add End of Index Entry (EOIE) extension, 2018-10-10) Git defaults to writing the new EOIE section when writing out an index file. Usually that is a good thing because it improves threaded performance, but when a Git repository is shared with older versions of Git, it produces a confusing warning:$ git status ignoring EOIE extension HEAD detached at 371ed0defa nothing to commit, working tree cleanLet's introduce the new index extension more gently. First we'll roll out the new version of Git that understands it, and then once sufficiently many users are using such a version, we can flip the default to writing it by default.
Quoted 9 lines
Quoted 2 lines
Introduce a '[index] recordEndOfIndexEntries' configuration variable to allow interested users to benefit from this index extension early.
Thanks. I am in principle OK with this approach. In fact, I suspect that the default may want to be dynamically determined, and we give this knob to let the users further force their preference. When no extension that benefits from multi-threading is written, the default can stay "no" in future versions of Git, for example.
While I can understand the user confusion the warning about ignoring an extension could cause I guess I'm a little surprised that people would see it that often. To see the warning means they are running a new version of git in the same repo as they are running an old version of git. I just haven't ever experienced that (I only ever have one copy of git installed) so am surprised it comes up often enough to warrant this change.
That said, if it _is_ that much of an issue, this patch makes sense and provides a way to more gracefully transition into this feature. Even if we had some logic to dynamically determine whether to write it or not, we'd still want to avoid confusing users when it did get written out.
Quoted 77 lines
Documentation/config.txt
Quoted 2 lines
Documentation/config.txt +0
-0The timing is a bit unfortunate for any topic to touch this file, and contrib/cocci would not help us in this case X-<.
Quoted 6 lines
read-cache.c +0
-0@@ -2698,6 +2698,15 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfilerollback_lock_file (lockfile);read-cache.c void update_index_if_able(struct index_state *istate, struct lock_file *lockfile · line 2708
Quoted 10 lines
read-cache.c +8
-0} +static int record_eoie (void ) +{ +int val; + +if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ; +}Unconditionally defaulting to no in this round is perfectly fine. Let's make a mental note that this is the place to decide dynamic default in the future when we want to. It would probably have to ask around various "extension writing" helpers if they want to have a say in the outcome (e.g. if there are very many cache entries in the istate, the entry offset table may want to be written and otherwise not).
Quoted 14 lines
read-cache.c +1
-1@@ -2945,7 +2954,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,* read. Write it out regardless of the strip_extensions parameter as we need it * when loading the shared index. */ -if (offset) { +if (offset &&record_eoie ()) {struct strbuf sb =STRBUF_INIT ;write_eoie_extension (&sb, &eoie_c, offset);t/t1700-split-index.sh +0
-0t/t1700-split-index.sh test_expect_success 'enable split index' ' · line 29
Quoted 5 lines
t/t1700-split-index.sh +2
-0@@ -25,14 +25,17 @@ test_expect_success 'enable split index' ' git update-index --split-index && test-tool dump-split-index .git/index >actual && indexversion=$(test-tool index-version <.git/index) && + + # NEEDSWORK: Stop hard-coding checksums.Also let's stop hard-coding the assumption that the new knob is off by default. Ideally, you'd want to test both cases, right?
Perhaps you'd call "git update-index --split-index" we see in the precontext twice, with "-c VAR=false" and "-c VAR=true", to prepare "actual.without-eoie" and "actual.with-eoie", or something like that?
Thanks.
Quoted 16 lines
t/t1700-split-index.sh +5
-4if test "$indexversion" = "4" then - own=3527df833c6c100d3d1d921a9a782d62a8be4b58 - base=746f7ab2ed44fb839efdfbffcf399d0b113fb4cb + own=432ef4b63f32193984f339431fd50ca796493569 + base=508851a7f0dfa8691e9f69c7f055865389012491 else - own=5e9b60117ece18da410ddecc8b8d43766a0e4204 - base=4370042739b31cd17a5c5cd6043a77c9a00df113 + own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339 + base=39d890139ee5356c7ef572216cebcd27aa41f9df fi && + cat >expect <<-EOF && own $own base $base
Re: [PATCH 1/3] eoie: default to not writing EOIE section
Hi,
Quoted 6 lines · Ben Peart
While I can understand the user confusion the warning about ignoring an extension could cause I guess I'm a little surprised that people would see it that often. To see the warning means they are running a new version of git in the same repo as they are running an old version of git. I just haven't ever experienced that (I only ever have one copy of git installed) so am surprised it comes up often enough to warrant this change.
Great question. There are a few contexts where it comes up:
1. Using multiple versions of Git on a single machine. For example, some IDEs bundle a particular version of Git, which can be a different version from the system copy, or on a Mac, /usr/bin/git quickly goes out of sync with the Homebrew git in /usr/local/bin/git.
2. Sharing a single Git repository between multiple machines. This is not unusual, using NFS or sshfs, for example.
3. Downgrading after trying a new version of Git.
To support these, Git is generally careful to avoid writing repositories that older versions of Git do not understand. The EOIE extension was almost perfect in this respect: it works fine with older versions of Git, except for the alarming error message.
Quoted 4 lines
That said, if it _is_ that much of an issue, this patch makes sense and provides a way to more gracefully transition into this feature. Even if we had some logic to dynamically determine whether to write it or not, we'd still want to avoid confusing users when it did get written out.
Yes. An earlier version of this patch defaulted to writing EOIE if and only if the .git/index file already has an EOIE extension. There were enough holes in that (commands like "git reset" that do not read the existing index file) and enough complexity that it didn't seem worth it.
Really in this series, patch 3/3 is the one I care most about. I wish we had had it years ago. :) It would make patches 1 and 2 unnecessary.
Thanks,
Jonathan
Re: [PATCH 1/3] eoie: default to not writing EOIE section
Quoted 5 lines · Jonathan Nieder
1. Using multiple versions of Git on a single machine. For example, some IDEs bundle a particular version of Git, which can be a different version from the system copy, or on a Mac,
/usr/bin/gitquickly goes out of sync with the Homebrew git in/usr/local/bin/git.
Exactly this, especially the latter, is the answer to your question in an earlier message:
Quoted 2 lines
Quoted 2 lines
Am I understanding correclty? Can you give an example of when a user would *want* to see this message and what they would do in response?
The user may not be even aware of using another version of Git that does not know how to take advantage of the version of Git you have used in the repository, and it can be a mistake the user may want to fix (e.g. by futzing with PATH). The message would help the user notice the situation and take corrective action. Users of IDEs that bundle stale version of Git cannot even bug the supplier of the IDE to make them more up-to-date if they aren't aware of it.
Re: [PATCH 1/3] eoie: default to not writing EOIE section
Quoted 11 lines · Junio C Hamano
Quoted 5 lines · Jonathan Nieder
1. Using multiple versions of Git on a single machine. For example, some IDEs bundle a particular version of Git, which can be a different version from the system copy, or on a Mac,
/usr/bin/gitquickly goes out of sync with the Homebrew git in/usr/local/bin/git.Exactly this, especially the latter, is the answer to your question in an earlier message:
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Am I understanding correctly? Can you give an example of when a user would *want* to see this message and what they would do in response?
The user may not be even aware of using another version of Git that does not know how to take advantage of the version of Git you have used in the repository, and it can be a mistake the user may want to fix (e.g. by futzing with PATH).
Ah, thanks much. I'll add a hint along those lines (e.g.
warning: ignoring optional IEOT index extension
hint: This is likely due to the file having been written by a newer
hint: version of Git than is reading it. You can upgrade Git to
hint: take advantage of performance improvements from the updated
hint: file format.
hint:
hint: You can run "git config advice.unknownIndexExtension true" to
hint: suppress this message.
I am still vaguely uncomfortable with this since it seems analogous to warning that the server is advertising an unrecognized capability, but I can live with it. :)
Patch coming in a few moments.
Jonathan
PATCH 2/3 ieot: default to not writing IEOT section
In reply to Jonathan NiederAs with EOIE, popular versions of Git do not support the new IEOT extension yet. When accessing a Git repository written by a more modern version of Git, they correctly ignore the unrecognized section, but in the process they loudly warn
ignoring IEOT extension
resulting in confusion for users. Introduce the index extension more gently by not writing it yet in this first version with support for it. Soon, once sufficiently many users are running a modern version of Git, we can flip the default so users benefit from this index extension by default.
Introduce a '[index] recordOffsetTable' configuration variable to control whether the new index extension is written.
--- Documentation/config.txt | 7 +++++++ read-cache.c | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-)
Documentation/config.txt +7 -0
@@ -2195,6 +2195,13 @@ index.recordEndOfIndexEntries:: reading the index using Git versions before 2.20. Defaults to 'false'. +index.recordOffsetTable:: + Specifies whether the index file should include an "Index Entry + Offset Table" section. This reduces index load time on + multiprocessor machines but produces a message "ignoring IEOT + extension" when reading the index using Git versions before 2.20. + Defaults to 'false'. + index.threads:: Specifies the number of threads to spawn when loading the index. This is meant to reduce index load time on multiprocessor machines.
read-cache.c +10 -1
@@ -2707,6 +2707,15 @@ static int record_eoie(void)return 0 ; } +static int record_ieot (void ) +{ +int val; + +if (!git_config_get_bool ("index.recordoffsettable" , &val)) +return val; +return 0 ; +} +/* * On success, `tempfile` is closed. If it is the temporary file * of a `struct lock_file`, we will therefore effectively perform
@@ -2767,7 +2776,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,#ifndef NO_PTHREADS nr_threads =git_config_get_index_threads (); -if (nr_threads !=1 ) { +if (nr_threads !=1 &&record_ieot ()) {int ieot_blocks, cpus;/*
Signature · 1 line
2.19.1.930.g4563a0d9d0
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Documentation/config.txt index.recordEndOfIndexEntries:: · line 2203
Quoted 6 lines
Documentation/config.txt +6
-0+index.recordOffsetTable:: + Specifies whether the index file should include an "Index Entry + Offset Table" section. This reduces index load time on + multiprocessor machines but produces a message "ignoring IEOT + extension" when reading the index using Git versions before 2.20. + Defaults to 'false'.
Probably worth adding a test that exercises this new config option - somehow create an index with index.recordOffsetTable=1, check that the index contains the appropriate string (a few ways to do this, but I'm not sure which are portable), and then run a Git command that reads the index to make sure it is valid; then do the same except index.recordOffsetTable=0.
The code itself looks good to me.
Same comment for patch 1.
Re: [PATCH 2/3] ieot: default to not writing IEOT section
In reply to Jonathan NiederQuoted 8 lines · Jonathan Nieder
As with EOIE, popular versions of Git do not support the new IEOT extension yet. When accessing a Git repository written by a more modern version of Git, they correctly ignore the unrecognized section, but in the process they loudly warn
ignoring IEOT extension
resulting in confusion for users.
Then removing the message is throwing it with bathwater. First think about which part of the message is confusiong and then make it less confusing.
How about
hint: ignoring an optional IEOT extension
to make it clear that it is totally harmless?
With that, we can add advise.unknownIndexExtension=false to turn all of them off with a single switch.
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Quoted 8 lines · Junio C Hamano
How about
hint: ignoring an optional IEOT extension
to make it clear that it is totally harmless?
With that, we can add advise.unknownIndexExtension=false to turn all of them off with a single switch.
I like it. Expect a patch soon (tonight or tomorrow) that does that.
We'll have to find some appropriate place in the documentation to explain what the message is about, still.
Thanks,
Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Quoted 9 lines · Jonathan Nieder
Quoted 6 lines · Junio C Hamano
How about
hint: ignoring an optional IEOT extension
to make it clear that it is totally harmless?
Quoted 7 lines
Quoted 2 lines
With that, we can add advise.unknownIndexExtension=false to turn all of them off with a single switch.
I like it. Expect a patch soon (tonight or tomorrow) that does that.
We'll have to find some appropriate place in the documentation to explain what the message is about, still.
Also from the last discussion, if I remember correctly, this "ignoring" is considered harmless and could be suppressed most of the time. But commands like 'fsck' should always report it.
Signature · 1 line
Duy
Re: [PATCH 2/3] ieot: default to not writing IEOT section
In reply to Junio C HamanoHi again,
Quoted 4 lines · Junio C Hamano
Then removing the message is throwing it with bathwater. First think about which part of the message is confusiong and then make it less confusing.
Quoted 8 lines
How about
hint: ignoring an optional IEOT extension
to make it clear that it is totally harmless?
With that, we can add advise.unknownIndexExtension=false to turn all of them off with a single switch.
After having slept on it, this doesn't seem like a good fit for the advice subsystem. The advice subsystem provides hints about suggested actions for new users to understand what to do about a condition. In this example, the message is not suggesting a particular user action --- instead, it's describing state, which would seem to be a better fit for tracing, as in the patch 3/3 I sent.
Am I understanding correclty? Can you give an example of when a user would *want* to see this message and what they would do in response?
Thanks,
Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
In reply to Jonathan NiederQuoted 7 lines · Jonathan Nieder
As with EOIE, popular versions of Git do not support the new IEOT extension yet. When accessing a Git repository written by a more modern version of Git, they correctly ignore the unrecognized section, but in the process they loudly warn
ignoring IEOT extension
Quoted 9 lines
resulting in confusion for users. Introduce the index extension more gently by not writing it yet in this first version with support for it. Soon, once sufficiently many users are running a modern version of Git, we can flip the default so users benefit from this index extension by default.
Introduce a '[index] recordOffsetTable' configuration variable to control whether the new index extension is written.
Why introduce a new setting to disable writing the IEOT extension instead of just using the existing index.threads setting? If index.threads=1 then the IEOT extension isn't written which (I believe) will accomplish the same goal.
Quoted 54 lines
Signed-off-by Jonathan Nieder <[email protected]>--- Documentation/config.txt | 7 +++++++ read-cache.c | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-)Documentation/config.txt +7
-0@@ -2195,6 +2195,13 @@ index.recordEndOfIndexEntries:: reading the index using Git versions before 2.20. Defaults to 'false'. +index.recordOffsetTable:: + Specifies whether the index file should include an "Index Entry + Offset Table" section. This reduces index load time on + multiprocessor machines but produces a message "ignoring IEOT + extension" when reading the index using Git versions before 2.20. + Defaults to 'false'. + index.threads:: Specifies the number of threads to spawn when loading the index. This is meant to reduce index load time on multiprocessor machines.read-cache.c +10
-1@@ -2707,6 +2707,15 @@ static int record_eoie(void)return 0 ; } +static int record_ieot (void ) +{ +int val; + +if (!git_config_get_bool ("index.recordoffsettable" , &val)) +return val; +return 0 ; +} +/* * On success, `tempfile` is closed. If it is the temporary file * of a `struct lock_file`, we will therefore effectively perform @@ -2767,7 +2776,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,#ifndef NO_PTHREADS nr_threads =git_config_get_index_threads (); -if (nr_threads !=1 ) { +if (nr_threads !=1 &&record_ieot ()) {int ieot_blocks, cpus;/*
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Hi,
Quoted 12 lines · Ben Peart
Quoted 10 lines · Jonathan Nieder
As with EOIE, popular versions of Git do not support the new IEOT extension yet. When accessing a Git repository written by a more modern version of Git, they correctly ignore the unrecognized section, but in the process they loudly warn
ignoring IEOT extension
resulting in confusion for users. Introduce the index extension more gently by not writing it yet in this first version with support for it.
[...]
Quoted 7 lines
Quoted 2 lines
Introduce a '[index] recordOffsetTable' configuration variable to control whether the new index extension is written.
Why introduce a new setting to disable writing the IEOT extension instead of just using the existing index.threads setting? If index.threads=1 then the IEOT extension isn't written which (I believe) will accomplish the same goal.
Do you mean defaulting to index.threads=1? I don't think that would be a good default, but if you have a different change in mind then I'd be happy to hear it.
Or do you mean that if the user has explicitly specified index.threads=true, then that should imply index.recordOffsetTable=true so users only have to set one setting to turn it on? I can imagine that working well.
Thanks,
Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Quoted 24 lines · Jonathan Nieder
Hi,
Quoted 12 lines · Ben Peart
Quoted 10 lines · Jonathan Nieder
As with EOIE, popular versions of Git do not support the new IEOT extension yet. When accessing a Git repository written by a more modern version of Git, they correctly ignore the unrecognized section, but in the process they loudly warn
ignoring IEOT extension
resulting in confusion for users. Introduce the index extension more gently by not writing it yet in this first version with support for it.
[...]
Quoted 7 lines
Quoted 2 lines
Introduce a '[index] recordOffsetTable' configuration variable to control whether the new index extension is written.
Why introduce a new setting to disable writing the IEOT extension instead of just using the existing index.threads setting? If index.threads=1 then the IEOT extension isn't written which (I believe) will accomplish the same goal.
Quoted 8 lines
Do you mean defaulting to index.threads=1? I don't think that would be a good default, but if you have a different change in mind then I'd be happy to hear it.
Or do you mean that if the user has explicitly specified index.threads=true, then that should imply index.recordOffsetTable=true so users only have to set one setting to turn it on? I can imagine that working well.
Reading the index with multiple threads requires the EOIE and IEOT extensions to exist in the index. If either extension doesn't exist, then the code falls back to the single threaded path. That means you can't have both 1) no warning for old versions of git and 2) multi-threaded reading for new versions of git.
If you set index.threads=1, that will prevent the IEOT extension from being written and there will be no "ignoring IEOT extension" warning in older versions of git.
With this patch 'as is' you would have to set both index.threads=true and index.recordOffsetTable=true to get multi-threaded index reads. If either is set to false, it will silently drop back to single threaded reads.
Quoted 3 lines
Thanks,
Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Hi again,
Quoted 22 lines · Ben Peart
Quoted 7 lines · Jonathan Nieder
Quoted 4 lines · Ben Peart
Why introduce a new setting to disable writing the IEOT extension instead of just using the existing index.threads setting? If index.threads=1 then the IEOT extension isn't written which (I believe) will accomplish the same goal.
Quoted 7 lines
Do you mean defaulting to index.threads=1? I don't think that would be a good default, but if you have a different change in mind then I'd be happy to hear it.
Or do you mean that if the user has explicitly specified index.threads=true, then that should imply index.recordOffsetTable=true so users only have to set one setting to turn it on? I can imagine that working well.
Reading the index with multiple threads requires the EOIE and IEOT extensions to exist in the index. If either extension doesn't exist, then the code falls back to the single threaded path. That means you can't have both 1) no warning for old versions of git and 2) multi-threaded reading for new versions of git.
Quoted 7 lines
If you set index.threads=1, that will prevent the IEOT extension from being written and there will be no "ignoring IEOT extension" warning in older versions of git.
With this patch 'as is' you would have to set both index.threads=true and index.recordOffsetTable=true to get multi-threaded index reads. If either is set to false, it will silently drop back to single threaded reads.
Sorry, I'm still not understanding what you're proposing. What would be
- the default behavior
- the mechanism for changing that behavior
under your proposal?
I consider index.threads=1 to be a bad default. I would understand if you are saying that that should be the default, and I tried to propose a different way to achieve what you're looking for in the quoted reply above (but I'm not understanding whether you like that proposal or not).
Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Quoted 38 lines · Jonathan Nieder
Hi again,
Quoted 22 lines · Ben Peart
Quoted 7 lines · Jonathan Nieder
Quoted 4 lines · Ben Peart
Why introduce a new setting to disable writing the IEOT extension instead of just using the existing index.threads setting? If index.threads=1 then the IEOT extension isn't written which (I believe) will accomplish the same goal.
Quoted 7 lines
Do you mean defaulting to index.threads=1? I don't think that would be a good default, but if you have a different change in mind then I'd be happy to hear it.
Or do you mean that if the user has explicitly specified index.threads=true, then that should imply index.recordOffsetTable=true so users only have to set one setting to turn it on? I can imagine that working well.
Reading the index with multiple threads requires the EOIE and IEOT extensions to exist in the index. If either extension doesn't exist, then the code falls back to the single threaded path. That means you can't have both 1) no warning for old versions of git and 2) multi-threaded reading for new versions of git.
Quoted 7 lines
If you set index.threads=1, that will prevent the IEOT extension from being written and there will be no "ignoring IEOT extension" warning in older versions of git.
With this patch 'as is' you would have to set both index.threads=true and index.recordOffsetTable=true to get multi-threaded index reads. If either is set to false, it will silently drop back to single threaded reads.
Sorry, I'm still not understanding what you're proposing. What would be
- the default behavior
- the mechanism for changing that behavior
Quoted 8 lines
under your proposal?
I consider index.threads=1 to be a bad default. I would understand if you are saying that that should be the default, and I tried to propose a different way to achieve what you're looking for in the quoted reply above (but I'm not understanding whether you like that proposal or not).
Today, both the write logic (ie should we write out the IEOT extension) and the read logic (should I use the IEOT, if available, and do multi-threaded reading) are controlled by the single "index.threads" setting. I would like to keep the settings as simple as possible to prevent user confusion.
If we have two different settings (index.threads and index.recordoffsettable) then the only combination that will result in the user actually getting multi-threaded reads is when they are both set to true. Any other combination will silently fail. I think it would be confusing if you set index.threads=true and got no error message but didn't get multi-threaded reads either (or vice versa).
If you want to prevent any of the scary "ignoring IEOT extension" from ever happening then your only option is to turn off the IEOT writing by default. The downside is that people have to discover and turn it on if they want the improvements. This can be achieved by changing the default for index.threads from "true" to "false."
config.c +1 -1
@@ -2291,7 +2291,7 @@ int git_config_get_fsmonitor(void)int git_config_get_index_threads (void ) { -int is_bool, val =0 ; +int is_bool, val =1 ; val =git_env_ulong ("GIT_TEST_INDEX_THREADS" ,0 );if (val)
If you want to provide a way for a concerned user to disable the message after the first time they have seen it, then they can be instructed to run 'git config --global index.threads false'
There is no way to get multi-threaded reads and NOT get the scary message with older versions of git. Multi-threaded reads require the IEOT extension to be written into the index and the existence of the IEOT extension in the index will always generate the scary warning.
Quoted 2 lines
Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Hi,
Quoted 4 lines · Ben Peart
There is no way to get multi-threaded reads and NOT get the scary message with older versions of git. Multi-threaded reads require the IEOT extension to be written into the index and the existence of the IEOT extension in the index will always generate the scary warning.
This is where I think we differ. I want my local copy of Git to get multi-threaded reads as long as IEOT happens to be there, even if I am not ready to write IEOT myself yet.
I understand that this differs from what you would prefer, so I'd like to find some compromise that makes us both happy. I've tried to suggest one:
Make explicitly setting index.threads=true imply index.recordOffsetTable=true. That way, the default behavior is the behavior I prefer, and a client can simply set index.threads=true to get the behavior I think you are describing preferring.
My preference is instead what I sent in patch 2/3 (for simplicity, especially since the default of index.recordOffsetTable=false would be only temporary), but this would work okay for me.
I'll send this as a patch. If there is a reason it won't work for you, I would be very happy to learn more about why.
Thanks,
Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
In reply to Ben PeartQuoted 2 lines · Ben Peart
Why introduce a new setting to disable writing the IEOT extension instead of just using the existing index.threads setting?
But index.threads is about what the reader does, not about the writer who does not even know who will be reading the resulting index, no?
v2 cover Avoid confusing messages from new index extensions
Quoted 8 lines · Junio C Hamano
Quoted 2 lines · Ben Peart
Why introduce a new setting to disable writing the IEOT extension instead of just using the existing index.threads setting?
But index.threads is about what the reader does, not about the writer who does not even know who will be reading the resulting index, no?
It affects the writer, too, since it affects the number of blocks, but from an end user's point of view, I agree.
Here's an updated version of the series.
Patches 1-3 are as before, except that they are rebased to avoid conflicting with nd/config-split.
Patch 4 allows enabling the new index extensions with a single config setting, to address the feedback above.
Patch 5 revives the noisiness when encountering an unknown index extension, guarded with an advice setting.
Sorry for the delay in getting this out. Thoughts of all kinds welcome, as always.
Sincerely,
Jonathan Nieder (5):
eoie: default to not writing EOIE section
ieot: default to not writing IEOT section
index: do not warn about unrecognized extensions
index: make index.threads=true enable ieot and eoie
index: offer advice for unknown index extensions
Documentation/config/index.txt | 16 ++++++++++ advice.c | 2 ++ advice.h | 1 + config.c | 17 ++++++----- config.h | 2 +- read-cache.c | 54 +++++++++++++++++++++++++++++----- t/t1700-split-index.sh | 11 ++++--- 7 files changed, 84 insertions(+), 19 deletions(-)
PATCH v2 1/5 eoie: default to not writing EOIE section
Since 3b1d9e04 (eoie: add End of Index Entry (EOIE) extension, 2018-10-10) Git defaults to writing the new EOIE section when writing out an index file. Usually that is a good thing because it improves threaded performance, but when a Git repository is shared with older versions of Git, it produces a confusing warning:
$ git status ignoring EOIE extension HEAD detached at 371ed0defa nothing to commit, working tree clean
Let's introduce the new index extension more gently. First we'll roll out the new version of Git that understands it, and then once sufficiently many users are using such a version, we can flip the default to writing it by default.
Introduce a '[index] recordEndOfIndexEntries' configuration variable to allow interested users to benefit from this index extension early.
---
Rebased. No other change from v1.
As Jonathan pointed out, it would be nice to have tests here. Ben, any advice for how I could write some in a followup change? E.g. does Derrick Stolee's tracing based testing trick apply here?
Documentation/config/index.txt | 7 +++++++ read-cache.c | 11 ++++++++++- t/t1700-split-index.sh | 11 +++++++---- 3 files changed, 24 insertions(+), 5 deletions(-)
Documentation/config/index.txt +7 -0
@@ -1,3 +1,10 @@ +index.recordEndOfIndexEntries:: + Specifies whether the index file should include an "End Of Index + Entry" section. This reduces index load time on multiprocessor + machines but produces a message "ignoring EOIE extension" when + reading the index using Git versions before 2.20. Defaults to + 'false'. + index.threads:: Specifies the number of threads to spawn when loading the index. This is meant to reduce index load time on multiprocessor machines.
read-cache.c +10 -1
@@ -2689,6 +2689,15 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfilerollback_lock_file (lockfile); } +static int record_eoie (void ) +{ +int val; + +if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ; +} +/* * On success, `tempfile` is closed. If it is the temporary file * of a `struct lock_file`, we will therefore effectively perform
@@ -2936,7 +2945,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,* read. Write it out regardless of the strip_extensions parameter as we need it * when loading the shared index. */ -if (offset) { +if (offset &&record_eoie ()) {struct strbuf sb =STRBUF_INIT ;write_eoie_extension (&sb, &eoie_c, offset);
t/t1700-split-index.sh +7 -4
@@ -25,14 +25,17 @@ test_expect_success 'enable split index' ' git update-index --split-index && test-tool dump-split-index .git/index >actual && indexversion=$(test-tool index-version <.git/index) && + + # NEEDSWORK: Stop hard-coding checksums. if test "$indexversion" = "4" then - own=3527df833c6c100d3d1d921a9a782d62a8be4b58 - base=746f7ab2ed44fb839efdfbffcf399d0b113fb4cb + own=432ef4b63f32193984f339431fd50ca796493569 + base=508851a7f0dfa8691e9f69c7f055865389012491 else - own=5e9b60117ece18da410ddecc8b8d43766a0e4204 - base=4370042739b31cd17a5c5cd6043a77c9a00df113 + own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339 + base=39d890139ee5356c7ef572216cebcd27aa41f9df fi && + cat >expect <<-EOF && own $own base $base
Signature · 1 line
2.20.0.rc0.387.gc7a69e6b6c
Re: [PATCH 1/5] eoie: default to not writing EOIE section
Quoted 52 lines · Jonathan Nieder
Since
3b1d9e04(eoie: add End of Index Entry (EOIE) extension, 2018-10-10) Git defaults to writing the new EOIE section when writing out an index file. Usually that is a good thing because it improves threaded performance, but when a Git repository is shared with older versions of Git, it produces a confusing warning:$ git status ignoring EOIE extension HEAD detached at 371ed0defa nothing to commit, working tree cleanLet's introduce the new index extension more gently. First we'll roll out the new version of Git that understands it, and then once sufficiently many users are using such a version, we can flip the default to writing it by default.
Introduce a '[index] recordEndOfIndexEntries' configuration variable to allow interested users to benefit from this index extension early.
Signed-off-by Jonathan Nieder <[email protected]>---
Rebased. No other change from v1.As Jonathan pointed out, it would be nice to have tests here. Ben, any advice for how I could write some in a followup change? E.g. does Derrick Stolee's tracing based testing trick apply here?
Documentation/config/index.txt | 7 +++++++ read-cache.c | 11 ++++++++++- t/t1700-split-index.sh | 11 +++++++---- 3 files changed, 24 insertions(+), 5 deletions(-)Documentation/config/index.txt +7
-0@@ -1,3 +1,10 @@ +index.recordEndOfIndexEntries:: + Specifies whether the index file should include an "End Of Index + Entry" section. This reduces index load time on multiprocessor + machines but produces a message "ignoring EOIE extension" when + reading the index using Git versions before 2.20. Defaults to + 'false'. + index.threads:: Specifies the number of threads to spawn when loading the index. This is meant to reduce index load time on multiprocessor machines.read-cache.c +0
-0
read-cache.c void update_index_if_able(struct index_state *istate, struct lock_file *lockfile · line 2694
Quoted 6 lines
read-cache.c +3
-0@@ -2689,6 +2689,15 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfilerollback_lock_file (lockfile); } +static int record_eoie (void ) +{ +int val;
I believe you are going to want to initialize val to 0 here as it is on the stack so is not guaranteed to be zero.
Quoted 45 lines
read-cache.c +7
-1+ +if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ; +} +/* * On success, `tempfile` is closed. If it is the temporary file * of a `struct lock_file`, we will therefore effectively perform @@ -2936,7 +2945,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,* read. Write it out regardless of the strip_extensions parameter as we need it * when loading the shared index. */ -if (offset) { +if (offset &&record_eoie ()) {struct strbuf sb =STRBUF_INIT ;write_eoie_extension (&sb, &eoie_c, offset);t/t1700-split-index.sh +7
-4@@ -25,14 +25,17 @@ test_expect_success 'enable split index' ' git update-index --split-index && test-tool dump-split-index .git/index >actual && indexversion=$(test-tool index-version <.git/index) && + + # NEEDSWORK: Stop hard-coding checksums. if test "$indexversion" = "4" then - own=3527df833c6c100d3d1d921a9a782d62a8be4b58 - base=746f7ab2ed44fb839efdfbffcf399d0b113fb4cb + own=432ef4b63f32193984f339431fd50ca796493569 + base=508851a7f0dfa8691e9f69c7f055865389012491 else - own=5e9b60117ece18da410ddecc8b8d43766a0e4204 - base=4370042739b31cd17a5c5cd6043a77c9a00df113 + own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339 + base=39d890139ee5356c7ef572216cebcd27aa41f9df fi && + cat >expect <<-EOF && own $own base $base
Re: [PATCH 1/5] eoie: default to not writing EOIE section
Quoted 5 lines · Ben Peart
Quoted 5 lines
read-cache.c +0
-0@@ -2689,6 +2689,15 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile
Quoted 8 lines
read-cache.c void update_index_if_able(struct index_state *istate, struct lock_file *lockfile · line 2694
Quoted 5 lines
read-cache.c +3
-0rollback_lock_file (lockfile); } +static int record_eoie (void ) +{ +int val;I believe you are going to want to initialize val to 0 here as it is on the stack so is not guaranteed to be zero.
The git_config_get_bool() call below will initialize it anyway.
Quoted 5 lines
Quoted 5 lines
read-cache.c +5
-0+ +if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ; +}
Re: [PATCH 1/5] eoie: default to not writing EOIE section
Quoted 15 lines · SZEDER Gábor
Quoted 5 lines · Ben Peart
Quoted 5 lines
read-cache.c +0
-0@@ -2689,6 +2689,15 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfileQuoted 8 lines
read-cache.c void update_index_if_able(struct index_state *istate, struct lock_file *lockfile · line 2694
Quoted 5 lines
read-cache.c +3
-0rollback_lock_file (lockfile); } +static int record_eoie (void ) +{ +int val;I believe you are going to want to initialize val to 0 here as it is on the stack so is not guaranteed to be zero.
Quoted 1 line
The
git_config_get_bool()call below will initialize it anyway.
Yes, there are two ways to write this. With a conditional to initialize and return or to return the default, as we have here:
Quoted 3 lines
Quoted 3 lines
read-cache.c void update_index_if_able(struct index_state *istate, struct lock_file *lockfile · line 2698
Quoted 3 lines
read-cache.c +3
-0+if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ;
Or initialize the default ahead of time, and rely on the function not to modify it when the entry is missing:
int val = 0;
git_config_get_bool("index.whatever", &val);
return val;I think either is perfectly fine, but since I also had to look at it twice to make sure it was doing the right thing, I figured it is worth mentioning as a possible style/convention thing we may want to decide on.
-Peff
Re: [PATCH 1/5] eoie: default to not writing EOIE section
Quoted 10 lines · Jeff King
Yes, there are two ways to write this. With a conditional to initialize and return or to return the default, as we have here:
Quoted 3 lines
Quoted 3 lines
read-cache.c void update_index_if_able(struct index_state *istate, struct lock_file *lockfile · line 2698
Quoted 3 lines
read-cache.c +3
-0+if (!git_config_get_bool ("index.recordendofindexentries" , &val)) +return val; +return 0 ;Or initialize the default ahead of time, and rely on the function not to modify it when the entry is missing:
Quoted 8 lines
int val = 0; git_config_get_bool("index.whatever", &val); return val;I think either is perfectly fine, but since I also had to look at it twice to make sure it was doing the right thing, I figured it is worth mentioning as a possible style/convention thing we may want to decide on.
I too think either is fine, and both rely on the git_config_get_*() to modify the value return only when it sees that it is set.
I'd choose the latter when the default value is simple, as the reader does not have to even know what the return value from the git_config_get_*() function means to follow what is going on.
On the other hand, the former (i.e. the original by Jonathan) is more flexible, and it makes it possible to write a piece of code, which computes a default that is expensive to build only when necessary, in the most natural way. The readers do need to be aware of how the functin signals "I didn't get anything" with its return value, though.
I do not mind standardizing on the latter, though. A caller with an expensive default can initialize val to an impossible "sentinel" value that signals the fact that git_config_get_*() did not get anything, as long as the type has a natural sentinel (like -1 for a bool to signal "unset"), and code that comes either immediately after git_config_get_*() or much much later in the control flow can check for the sentinel to see if it needs to compute the expensive default.
Re: [PATCH 1/5] eoie: default to not writing EOIE section
In reply to Jonathan NiederQuoted 19 lines · Jonathan Nieder
Since
3b1d9e04(eoie: add End of Index Entry (EOIE) extension, 2018-10-10) Git defaults to writing the new EOIE section when writing out an index file. Usually that is a good thing because it improves threaded performance, but when a Git repository is shared with older versions of Git, it produces a confusing warning:$ git status ignoring EOIE extension HEAD detached at 371ed0defa nothing to commit, working tree cleanLet's introduce the new index extension more gently. First we'll roll out the new version of Git that understands it, and then once sufficiently many users are using such a version, we can flip the default to writing it by default.
Introduce a '[index] recordEndOfIndexEntries' configuration variable to allow interested users to benefit from this index extension early.
Quoted 8 lines
Signed-off-by Jonathan Nieder <[email protected]>---
Rebased. No other change from v1.As Jonathan pointed out, it would be nice to have tests here. Ben, any advice for how I could write some in a followup change? E.g. does Derrick Stolee's tracing based testing trick apply here?
I suppose a 'test-dump-eoie' could be written along the lines of test-dump-fsmonitor or test-dump-untracked-cache. Unlike those, there isn't much state to dump other than the existence of the extension and the offset. That could be used to test that the new settings are working properly.
PATCH v2 2/5 ieot: default to not writing IEOT section
In reply to Jonathan NiederAs with EOIE, popular versions of Git do not support the new IEOT extension yet. When accessing a Git repository written by a more modern version of Git, they correctly ignore the unrecognized section, but in the process they loudly warn
ignoring IEOT extension
resulting in confusion for users. Introduce the index extension more gently by not writing it yet in this first version with support for it. Soon, once sufficiently many users are running a modern version of Git, we can flip the default so users benefit from this index extension by default.
Introduce a '[index] recordOffsetTable' configuration variable to control whether the new index extension is written.
---
As with patch 1/5, no change from v1 other than rebasing.
Documentation/config/index.txt | 7 +++++++ read-cache.c | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-)
Documentation/config/index.txt +7 -0
@@ -5,6 +5,13 @@ index.recordEndOfIndexEntries:: reading the index using Git versions before 2.20. Defaults to 'false'. +index.recordOffsetTable:: + Specifies whether the index file should include an "Index Entry + Offset Table" section. This reduces index load time on + multiprocessor machines but produces a message "ignoring IEOT + extension" when reading the index using Git versions before 2.20. + Defaults to 'false'. + index.threads:: Specifies the number of threads to spawn when loading the index. This is meant to reduce index load time on multiprocessor machines.
read-cache.c +10 -1
@@ -2698,6 +2698,15 @@ static int record_eoie(void)return 0 ; } +static int record_ieot (void ) +{ +int val; + +if (!git_config_get_bool ("index.recordoffsettable" , &val)) +return val; +return 0 ; +} +/* * On success, `tempfile` is closed. If it is the temporary file * of a `struct lock_file`, we will therefore effectively perform
@@ -2761,7 +2770,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,else nr_threads =1 ; -if (nr_threads !=1 ) { +if (nr_threads !=1 &&record_ieot ()) {int ieot_blocks, cpus;/*
Signature · 1 line
2.20.0.rc0.387.gc7a69e6b6c-goog
Re: [PATCH 2/5] ieot: default to not writing IEOT section
Quoted 47 lines · Jonathan Nieder
As with EOIE, popular versions of Git do not support the new IEOT extension yet. When accessing a Git repository written by a more modern version of Git, they correctly ignore the unrecognized section, but in the process they loudly warn
ignoring IEOT extension
resulting in confusion for users. Introduce the index extension more gently by not writing it yet in this first version with support for it. Soon, once sufficiently many users are running a modern version of Git, we can flip the default so users benefit from this index extension by default.
Introduce a '[index] recordOffsetTable' configuration variable to control whether the new index extension is written.
Signed-off-by Jonathan Nieder <[email protected]>---
As with patch 1/5, no change from v1 other than rebasing.Documentation/config/index.txt | 7 +++++++ read-cache.c | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-)Documentation/config/index.txt +7
-0@@ -5,6 +5,13 @@ index.recordEndOfIndexEntries:: reading the index using Git versions before 2.20. Defaults to 'false'. +index.recordOffsetTable:: + Specifies whether the index file should include an "Index Entry + Offset Table" section. This reduces index load time on + multiprocessor machines but produces a message "ignoring IEOT + extension" when reading the index using Git versions before 2.20. + Defaults to 'false'. + index.threads:: Specifies the number of threads to spawn when loading the index. This is meant to reduce index load time on multiprocessor machines.read-cache.c +0
-0
read-cache.c static int record_eoie(void) · line 2704
Quoted 7 lines
read-cache.c +4
-0@@ -2698,6 +2698,15 @@ static int record_eoie(void)return 0 ; } +static int record_ieot (void ) +{ +int val; +
Initialize stack val to zero to ensure proper default.
Quoted 18 lines
read-cache.c +6
-1+if (!git_config_get_bool ("index.recordoffsettable" , &val)) +return val; +return 0 ; +} +/* * On success, `tempfile` is closed. If it is the temporary file * of a `struct lock_file`, we will therefore effectively perform @@ -2761,7 +2770,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,else nr_threads =1 ; -if (nr_threads !=1 ) { +if (nr_threads !=1 &&record_ieot ()) {int ieot_blocks, cpus;/*
Re: [PATCH 2/5] ieot: default to not writing IEOT section
Quoted 6 lines
read-cache.c static int record_eoie(void) · line 2704
Quoted 4 lines
read-cache.c +4
-0+static int record_ieot (void ) +{ +int val; +Initialize stack val to zero to ensure proper default.
I don't think that is needed here, as we only use val when we first write to it via git_config_get_bool.
Did you spot this via code review and thought of defensive programming or is there a tool that
has a false positive here?
Quoted 5 lines
Quoted 5 lines
read-cache.c +4
-0+if (!git_config_get_bool ("index.recordoffsettable" , &val)) +return val; +return 0 ; +}
Re: [PATCH 2/5] ieot: default to not writing IEOT section
Quoted 7 lines · Stefan Beller
Quoted 6 lines
read-cache.c static int record_eoie(void) · line 2704
Quoted 4 lines
read-cache.c +4
-0+static int record_ieot (void ) +{ +int val; +Initialize stack val to zero to ensure proper default.
Quoted 7 lines
I don't think that is needed here, as we only use
valwhen we first write to it viagit_config_get_bool.Did you spot this via code review and thought of defensive programming or is there a tool that
has a false positive here?
Code review and defensive programming. I had to review the code in git_config_get_bool() to see if it always initialized the val even if it didn't find the requested config variable (esp since we don't pass in a default value for this function like we do others).
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
read-cache.c +4
-0+if (!git_config_get_bool ("index.recordoffsettable" , &val)) +return val; +return 0 ; +}
Re: [PATCH 2/5] ieot: default to not writing IEOT section
Quoted 19 lines · Ben Peart
Quoted 7 lines · Stefan Beller
Quoted 6 lines
read-cache.c static int record_eoie(void) · line 2704
Quoted 4 lines
read-cache.c +4
-0+static int record_ieot (void ) +{ +int val; +Initialize stack val to zero to ensure proper default.
Quoted 7 lines
I don't think that is needed here, as we only use
valwhen we first write to it viagit_config_get_bool.Did you spot this via code review and thought of defensive programming or is there a tool that
has a false positive here?
Quoted 5 lines
Code review and defensive programming. I had to review the code in
git_config_get_bool()to see if it always initialized the val even if it didn't find the requested config variable (esp since we don't pass in a default value for this function like we do others).
Ah, sorry to have sent out this email, which I found as one of the earliest discussions in my mailbox. The later patches/discussions became a lot more heated from my cursory skimming and sorted out this as well.
It is interesting to notice that, as I also had to lookup how the config machinery works (once? a couple times?) but now it is so hardcoded in my brain to assume that if functions like git_config_* take the branch, we can access the value that the config function was supposed to read into.
Sorry for the noise,
Stefan
Re: [PATCH 2/5] ieot: default to not writing IEOT section
In reply to Stefan BellerQuoted 9 lines · Stefan Beller
Quoted 6 lines
read-cache.c static int record_eoie(void) · line 2704
Quoted 4 lines
read-cache.c +4
-0+static int record_ieot (void ) +{ +int val; +Initialize stack val to zero to ensure proper default.
I don't think that is needed here, as we only use
valwhen we first write to it viagit_config_get_bool.
Yup.
PATCH v2 3/5 index: do not warn about unrecognized extensions
In reply to Jonathan NiederDocumentation/technical/index-format explains:
4-byte extension signature. If the first byte is 'A'..'Z' the extension is optional and can be ignored.
This allows gracefully introducing a new index extension without having to rely on all readers having support for it. Mandatory extensions start with a lowercase letter and optional ones start with a capital. Thus the versions of Git acting on a shared local repository do not have to upgrade in lockstep.
We almost obey that convention, but there is a problem: when encountering an unrecognized optional extension, we write
ignoring FNCY extension
to stderr, which alarms users. This means that in practice we have had to introduce index extensions in two steps: first add read support, and then a while later, start writing by default. This delays when users can benefit from improvements to the index format.
We cannot change the past, but for index extensions of the future, there is a straightforward improvement: silence that message except when tracing. This way, the message is still available when debugging, but in everyday use it does not show up so (once most Git users have this patch) we can turn on new optional extensions right away without alarming people.
---
Unchanged.
read-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
read-cache.c +1 -1
@@ -1726,7 +1726,7 @@ static int read_index_extension(struct index_state *istate,if (*ext <'A' ||'Z' < *ext)return error ("index uses %.4s extension, which we do not understand" , ext); -fprintf (stderr,"ignoring %.4s extension\n" , ext); +trace_printf ("ignoring %.4s extension\n" , ext);break ; }return 0 ;
Signature · 1 line
2.20.0.rc0.387.gc7a69e6b6c
PATCH v2 4/5 index: make index.threads=true enable ieot and eoie
In reply to Jonathan NiederIf a user explicitly sets
[index] threads = true
to read the index using multiple threads, ensure that index writes include the offset table by default to make that possible. This ensures that the user's intent of turning on threading is respected.
In other words, permit the following configurations:
- index.threads and index.recordOffsetTable unspecified: do not write the offset table yet (to avoid alarming the user with "ignoring IEOT extension" messages when an older version of Git accesses the repository) but do make use of multiple threads to read the index if the supporting offset table is present.
This can also be requested explicitly by setting index.threads=true, 0, or >1 and index.recordOffsetTable=false.
- index.threads=false or 1: do not write the offset table, and do not make use of the offset table.
One can set index.recordOffsetTable=false as well, to be more explicit.
- index.threads=true, 0, or >1 and index.recordOffsetTable unspecified:
write the offset table and make use of threads at read time.
This can also be requested by setting index.threads=true, 0, >1, or unspecified and index.recordOffsetTable=true.
Fortunately the complication is temporary: once most Git installations have upgraded to a version with support for the IEOT and EOIE extensions, we can flip the defaults for index.recordEndOfIndexEntries and index.recordOffsetTable to true and eliminate the settings.
---
New, based on Ben Peart's feedback. Turned out simpler than I feared --- thanks, Ben, for pushing for this.
Documentation/config/index.txt | 6 ++++-- config.c | 17 ++++++++++------- config.h | 2 +- read-cache.c | 23 +++++++++++++++++------ 4 files changed, 32 insertions(+), 16 deletions(-)
Documentation/config/index.txt +4 -2
@@ -3,14 +3,16 @@ index.recordEndOfIndexEntries:: Entry" section. This reduces index load time on multiprocessor machines but produces a message "ignoring EOIE extension" when reading the index using Git versions before 2.20. Defaults to - 'false'. + 'true' if index.threads has been explicitly enabled, 'false' + otherwise. index.recordOffsetTable:: Specifies whether the index file should include an "Index Entry Offset Table" section. This reduces index load time on multiprocessor machines but produces a message "ignoring IEOT extension" when reading the index using Git versions before 2.20. - Defaults to 'false'. + Defaults to 'true' if index.threads has been explicitly enabled, + 'false' otherwise. index.threads:: Specifies the number of threads to spawn when loading the index.
config.c +10 -7
@@ -2294,22 +2294,25 @@ int git_config_get_fsmonitor(void)return 0 ; } -int git_config_get_index_threads (void ) +int git_config_get_index_threads (int *dest) { -int is_bool, val =0 ; +int is_bool, val; val =git_env_ulong ("GIT_TEST_INDEX_THREADS" ,0 ); -if (val) -return val; +if (val) { + *dest = val; +return 0 ; + }if (!git_config_get_bool_or_int ("index.threads" , &is_bool, &val)) {if (is_bool) -return val ?0 :1 ; + *dest = val ?0 :1 ;else -return val; + *dest = val; +return 0 ; } -return 0 ;/* auto */ +return 1 ; }NORETURN
config.h +1 -1
@@ -246,11 +246,11 @@ extern int git_config_get_bool(const char *key, int *dest);extern int git_config_get_bool_or_int (const char *key,int *is_bool,int *dest);extern int git_config_get_maybe_bool (const char *key,int *dest);extern int git_config_get_pathname (const char *key,const char **dest); +extern int git_config_get_index_threads (int *dest);extern int git_config_get_untracked_cache (void );extern int git_config_get_split_index (void );extern int git_config_get_max_percent_split_change (void );extern int git_config_get_fsmonitor (void ); -extern int git_config_get_index_threads (void );/* This dies if the configured or default date is in the future */ extern int git_config_get_expiry (const char *key,const char **output);
read-cache.c +17 -6
@@ -2176,7 +2176,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) src_offset =sizeof (*hdr); - nr_threads =git_config_get_index_threads (); +if (git_config_get_index_threads (&nr_threads)) + nr_threads =1 ;/* TODO: does creating more threads than cores help? */ if (!nr_threads) {
@@ -2695,7 +2696,13 @@ static int record_eoie(void)if (!git_config_get_bool ("index.recordendofindexentries" , &val))return val; -return 0 ; + +/* +* As a convenience, the end of index entries extension +* used for threading is written by default if the user +* explicitly requested threaded index reads. +*/ +return !git_config_get_index_threads (&val) && val !=1 ; }static int record_ieot (void )
@@ -2704,7 +2711,13 @@ static int record_ieot(void)if (!git_config_get_bool ("index.recordoffsettable" , &val))return val; -return 0 ; + +/* +* As a convenience, the offset table used for threading is +* written by default if the user explicitly requested +* threaded index reads. +*/ +return !git_config_get_index_threads (&val) && val !=1 ; }/*
@@ -2765,9 +2778,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,if (ce_write (&c, newfd, &hdr,sizeof (hdr)) <0 )return -1 ; -if (HAVE_THREADS ) - nr_threads =git_config_get_index_threads (); -else +if (!HAVE_THREADS ||git_config_get_index_threads (&nr_threads)) nr_threads =1 ;if (nr_threads !=1 &&record_ieot ()) {
Signature · 1 line
2.20.0.rc0.387.gc7a69e6b6c
Re: [PATCH 4/5] index: make index.threads=true enable ieot and eoie
Quoted 29 lines · Jonathan Nieder
If a user explicitly sets
[index] threads = trueto read the index using multiple threads, ensure that index writes include the offset table by default to make that possible. This ensures that the user's intent of turning on threading is respected.
In other words, permit the following configurations:
- index.threads and index.recordOffsetTable unspecified: do not write the offset table yet (to avoid alarming the user with "ignoring IEOT extension" messages when an older version of Git accesses the repository) but do make use of multiple threads to read the index if the supporting offset table is present.
This can also be requested explicitly by setting index.threads=true, 0, or >1 and index.recordOffsetTable=false.
- index.threads=false or 1: do not write the offset table, and do not make use of the offset table.
One can set index.recordOffsetTable=false as well, to be more explicit.
- index.threads=true, 0, or >1 and index.recordOffsetTable unspecified:
write the offset table and make use of threads at read time.
Quoted 8 lines
This can also be requested by setting index.threads=true, 0, >1, or unspecified and index.recordOffsetTable=true.
Fortunately the complication is temporary: once most Git installations have upgraded to a version with support for the IEOT and EOIE extensions, we can flip the defaults for index.recordEndOfIndexEntries and index.recordOffsetTable to true and eliminate the settings.
This looks good. I think this provides good default behavior while enabling fine grained control to those who want/need it.
I'm looking forward to the day when we can turn it back on by default so that people can take advantage of the speed improvements.
PATCH v2 5/5 index: offer advice for unknown index extensions
In reply to Jonathan NiederIt is not unusual for multiple distinct versions of Git to act on a single repository. For example, some IDEs bundle a particular version of Git, which can be a different version from the system copy of Git, or on a Mac, /usr/bin/git quickly goes out of sync with the Homebrew git in /usr/local/bin/git.
When a newer version of Git writes an index file that an older version of Git does not know how to make full use of, this is a teaching opportunity. The user may not be aware of what version of Git they are using. Print an advice message to help the user to use the most full featured version of Git (e.g. by futzing with their PATH).
warning: ignoring optional IEOT index extension
hint: This is likely due to the file having been written by a newer
hint: version of Git than is reading it. You can upgrade Git to
hint: take advantage of performance improvements from the updated
hint: file format.
hint:
hint: You can run "git config advice.unknownIndexExtension false"
hint: to suppress this message.
This replaces the message
ignoring IEOT extension
that existed previously and did not provide enough detail for a user to act on it or suppress it.
---
New, based on Junio's hints about the message removed in patch 3/5.
That's the end of the series. Thanks for reading, and thanks again for your help so far.
advice.c | 2 ++ advice.h | 1 + read-cache.c | 11 +++++++++++ 3 files changed, 14 insertions(+)
advice.c +2 -0
@@ -24,6 +24,7 @@ int advice_add_embedded_repo = 1;int advice_ignored_hook =1 ;int advice_waiting_for_editor =1 ;int advice_graft_file_deprecated =1 ; +int advice_unknown_index_extension =1 ;int advice_checkout_ambiguous_remote_branch_name =1 ;static int advice_use_color =-1 ;
@@ -78,6 +79,7 @@ static struct { { "ignoredHook", &advice_ignored_hook }, { "waitingForEditor", &advice_waiting_for_editor }, { "graftFileDeprecated", &advice_graft_file_deprecated }, + { "unknownIndexExtension", &advice_unknown_index_extension }, { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name }, /* make this an alias for backward compatibility */
advice.h +1 -0
@@ -24,6 +24,7 @@ extern int advice_add_embedded_repo;extern int advice_ignored_hook;extern int advice_waiting_for_editor;extern int advice_graft_file_deprecated; +extern int advice_unknown_index_extension;extern int advice_checkout_ambiguous_remote_branch_name;int git_default_advice_config (const char *var,const char *value);
read-cache.c +11 -0
@@ -1727,6 +1727,17 @@ static int read_index_extension(struct index_state *istate,return error ("index uses %.4s extension, which we do not understand" , ext);trace_printf ("ignoring %.4s extension\n" , ext); +if (advice_unknown_index_extension) { +warning (_ ("ignoring optional %.4s index extension" ), ext); +advise (_ ("This is likely due to the file having been written by a newer\n" +"version of Git than is reading it. You can upgrade Git to\n" +"take advantage of performance improvements from the updated\n" +"file format.\n" +"\n" +"Run \"%s\"\n" +"to suppress this message." ), +"git config advice.unknownIndexExtension false" ); + }break ; }return 0 ;
Signature · 1 line
2.20.0.rc0.387.gc7a69e6b6c
Re: [PATCH 5/5] index: offer advice for unknown index extensions
On Tue, Nov 20 2018, Jonathan Nieder wrote:
Just commenting here on the end-state of this since it's easier than each patch at a time:
First, do we still need to be doing %.4s instead of just %s? It would be easier for translators / to understand what's going on if it were just %s. I.e. "this is the extension name" v.s. "this is the first 4 bytes of whatever it is...".
read-cache.c static int read_index_extension(struct index_state *istate, · line 1728
Quoted 2 lines
read-cache.c +0
-0return error ("index uses %.4s extension, which we do not understand" , ext);
Missing _(). Not the fault of this series, but something to fix while we're at it.
Also not the fault of this series, the "is this upper case" test is unportable, but this is probably the tip of the iceberg for git not working on EBCDIC systems.
This message should say something like "Index uses the mandatory %s extension" to clarify and distinguish it from the below. We don't understand the upper-case one either, but the important distinction is that one is mandatory, and the other can be dropped. The two messages should make this clear.
Also, having advice() for that case is even more valuable since we have a hard error at this point. So something like:
"This is likely due to the index having been written by a future version of Git. All-lowercase index extensions are mandatory, as opposed to optional all-uppercase ones which we'll drop with a warning if we see them".
read-cache.c static int read_index_extension(struct index_state *istate, · line 1731
Quoted 3 lines
read-cache.c +2
-0trace_printf ("ignoring %.4s extension\n" , ext); +if (advice_unknown_index_extension) { +warning (_ ("ignoring optional %.4s index extension" ), ext);
Should start with upper-case. Good that it says "optional".
read-cache.c static int read_index_extension(struct index_state *istate, · line 1735
Quoted 4 lines
read-cache.c +4
-0+ advise(_("This is likely due to the file having been written by a newer\n" + "version of Git than is reading it. You can upgrade Git to\n" + "take advantage of performance improvements from the updated\n" + "file format.\n"
Let's not promise performance improvements with this extension in a future version. We have no idea what the extension is, yeah right now it's going to be true for the extension that prompted this patch series, but may not be in the future. So just something like this for the last sentence:
You can try upgrading Git to use this new index format.
read-cache.c static int read_index_extension(struct index_state *istate, · line 1739
Quoted 4 lines
read-cache.c +4
-0+"\n" +"Run \"%s\"\n" + "to suppress this message."), + "git config advice.unknownIndexExtension false");
Somewhat of an aside, but if I grep:
git grep -C10 'git config advice\..*false' -- '*.[ch]'
There's a few existing examples of this, but the majority of advice() messages don't say in the message how you can turn these off. Do we think this a message users would especially like to turn off? I have the opposite impression, it's a one-off in most cases, although not in the case where an editor has an embedded git.
I think it would make sense to add this sort of thing to the advice() API, i.e.:
advice_with_config_hint(_("<message>"), "unknownIndexExtension");Which would then know how to consistently print this advice about how to turn off the warning.
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Quoted 27 lines · Ævar Arnfjörð Bjarmason
On Tue, Nov 20 2018, Jonathan Nieder wrote:
Just commenting here on the end-state of this since it's easier than each patch at a time:
First, do we still need to be doing %.4s instead of just %s? It would be easier for translators / to understand what's going on if it were just %s. I.e. "this is the extension name" v.s. "this is the first 4 bytes of whatever it is...".
Quoted 2 lines
return error("index uses %.4s extension, which we do not understand",
ext);Missing
_(). Not the fault of this series, but something to fix while we're at it.Also not the fault of this series, the "is this upper case" test is unportable, but this is probably the tip of the iceberg for git not working on EBCDIC systems.
This message should say something like "Index uses the mandatory %s extension" to clarify and distinguish it from the below. We don't understand the upper-case one either, but the important distinction is that one is mandatory, and the other can be dropped. The two messages should make this clear.
Quoted 8 lines
Also, having
advice()for that case is even more valuable since we have a hard error at this point. So something like:"This is likely due to the index having been written by a future version of Git. All-lowercase index extensions are mandatory, as opposed to optional all-uppercase ones which we'll drop with a warning if we see them".
I agree that we should have different messages for mandatory and optional extensions. I don't think we should try and educate the end user on the implementation detail that git makes lower cases mandatory and upper case optional (ie drop the 'All-lowercase..." part). They will never see the lower vs upper case difference and can't do anything about it anyway.
Quoted 17 lines
read-cache.c static int read_index_extension(struct index_state *istate, · line 1731
Quoted 3 lines
trace_printf("ignoring %.4s extension\n", ext);read-cache.c +2
-0+if (advice_unknown_index_extension) { +warning (_ ("ignoring optional %.4s index extension" ), ext);Should start with upper-case. Good that it says "optional".
read-cache.c static int read_index_extension(struct index_state *istate, · line 1735
Quoted 4 lines
read-cache.c +4
-0+ advise(_("This is likely due to the file having been written by a newer\n" + "version of Git than is reading it. You can upgrade Git to\n" + "take advantage of performance improvements from the updated\n" + "file format.\n"Let's not promise performance improvements with this extension in a future version. We have no idea what the extension is, yeah right now it's going to be true for the extension that prompted this patch series, but may not be in the future. So just something like this for the last sentence:
Quoted 1 line
You can try upgrading Git to use this new index format.
Agree - not all are guaranteed to be perf related.
Quoted 16 lines
read-cache.c static int read_index_extension(struct index_state *istate, · line 1739
Quoted 5 lines
read-cache.c +4
-0+"\n" +"Run \"%s\"\n" + "to suppress this message."), + "git config advice.unknownIndexExtension false");Somewhat of an aside, but if I grep:
git grep -C10 'git config advice\..*false' -- '*.[ch]'There's a few existing examples of this, but the majority of
advice()messages don't say in the message how you can turn these off. Do we think this a message users would especially like to turn off? I have the opposite impression, it's a one-off in most cases, although not in the case where an editor has an embedded git.
Quoted 8 lines
I think it would make sense to add this sort of thing to the
advice()API, i.e.:advice_with_config_hint(_("<message>"), "unknownIndexExtension");Which would then know how to consistently print this advice about how to turn off the warning.
I like this. I personally never knew you could turn of the "spent xxx seconds finding untracked files" advice until I worked on this patch series. This would help make that feature more discoverable.
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Quoted 15 lines · Ben Peart
Quoted 6 lines
This message should say something like "Index uses the mandatory %s extension" to clarify and distinguish it from the below. We don't understand the upper-case one either, but the important distinction is that one is mandatory, and the other can be dropped. The two messages should make this clear.
Quoted 8 lines
Also, having
advice()for that case is even more valuable since we have a hard error at this point. So something like:"This is likely due to the index having been written by a future version of Git. All-lowercase index extensions are mandatory, as opposed to optional all-uppercase ones which we'll drop with a warning if we see them".
Quoted 6 lines
I agree that we should have different messages for mandatory and optional extensions. I don't think we should try and educate the end user on the implementation detail that git makes lower cases mandatory and upper case optional (ie drop the 'All-lowercase..." part). They will never see the lower vs upper case difference and can't do anything about it anyway.
I agree that the "warn and continue" message should say "optional" (meaning: safe to ignore but you would want to take note) while "cannot continue" message should say something different.
I do not mind a more verbose error message when we saw unknown but required extension, but unlike the "warn and continue" case, the program will stop and die with such an error right there, so I am not sure if it is worth allowing to tone it down by putting some part of the verbosity behind the advise() mechanism.
Quoted 3 lines
Quoted 3 lines
read-cache.c static int read_index_extension(struct index_state *istate, · line 1731
Quoted 3 lines
trace_printf("ignoring %.4s extension\n", ext);read-cache.c +2
-0+if (advice_unknown_index_extension) { +warning (_ ("ignoring optional %.4s index extension" ), ext);
So from that point of view, the distinction between this message and this one
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
return error("index uses %.4s extension, which we do not understand",
ext);
is halfway there. The message needs to anticipate and answer an end-user reaction: "we do not understand" so what?
I am still puzzled by the insistence of 3/5 and this step that wants to kill the coalmine canary. But I am even more puzzled by the first two steps that want to disable the two optional extensions.
What's so different this time with the new optional extensions?
The other early optional extensions like cache-tree or resolve-undo were added unconditionally and by definition appeared much earlier in git-core than any other Git reimplementations. verbody who recorded the fact that s/he resolved merge conflicts got REUC, and we would have given warning when an older Git did not understand these extensions [*1*]. We knudged users to more modern Git by preparing the old Gits to warn when there are unknown extensions, either by upgrading their Git themselves, or by bugging their toolsmiths. Nobody complained to propose to rip the messages like this round. This series has a strong smell of pushing back by the toolsmiths who refuse to promptly upgrade to help their users, and that is why I do not feel entirely happy with this series.
[Footnote]
*1* A Git that did not understand TREE would have been silent, as
it was the first extension and that was the first time we became aware of the need to warn unknown extensions.
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Hi,
Quoted 6 lines · Junio C Hamano
I am still puzzled by the insistence of 3/5 and this step that wants to kill the coalmine canary. But I am even more puzzled by the first two steps that want to disable the two optional extensions.
What's so different this time with the new optional extensions?
Quoted 12 lines
The other early optional extensions like cache-tree or resolve-undo were added unconditionally and by definition appeared much earlier in git-core than any other Git reimplementations. Everbody who recorded the fact that s/he resolved merge conflicts got REUC, and we would have given warning when an older Git did not understand these extensions [*1*]. We knudged users to more modern Git by preparing the old Gits to warn when there are unknown extensions, either by upgrading their Git themselves, or by bugging their toolsmiths. Nobody complained to propose to rip the messages like this round. This series has a strong smell of pushing back by the toolsmiths who refuse to promptly upgrade to help their users, and that is why I do not feel entirely happy with this series.
I acknowledge your puzzlement. I'm not sure what to do about it.
There are a few significant differences from the REUC case:
1. This happens whenever the index is refreshed. REUC, as you mentioned, only affected resolutions of conflicted merges. So users ran into it less often.
2. I never ran into the REUC case. If I had, I would have sent the same patch then.
3. Time has passed and people's standards may have gone up.
I wish I had been around when the message was added in the first place, so that I could have provided the same feedback about the message then. But I do not think that that should be held against me. I'm describing a real user problem.
Are the commit messages unclear? Is there some missing use case that this version of the patch misses?
I don't *think* you intend to say "sure, you got user reports, but (those users are wrong | those users are not real | you are not interpreting those users correctly)", but that is what I am hearing. On the other hand, I don't want to discourage useful review feedback, and I think adding the advise() call was a real improvement. I'm just getting confused about why I am still not being heard.
Jonathan
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Quoted 5 lines · onathan Nieder
Quoted 3 lines · Junio C Hamano
I am still puzzled by the insistence of 3/5 and this step that wants to kill the coalmine canary. But I am even more puzzled by the first two steps that want to disable the two optional extensions.
[...]
Quoted 11 lines
I acknowledge your puzzlement. I'm not sure what to do about it.
There are a few significant differences from the REUC case:
1. This happens whenever the index is refreshed. REUC, as you mentioned, only affected resolutions of conflicted merges. So users ran into it less often.
2. I never ran into the REUC case. If I had, I would have sent the same patch then.
Quoted 6 lines
3. Time has passed and people's standards may have gone up.
I wish I had been around when the message was added in the first place, so that I could have provided the same feedback about the message then. But I do not think that that should be held against me. I'm describing a real user problem.
And to be clear, it is the first two patches that address the immediate user problem. Whatever improvements we make to the warning message today, we cannot retroactively change the other versions of Git that users are using that want to access the same repository.
Jonathan
Re: [PATCH 5/5] index: offer advice for unknown index extensions
In reply to Jonathan NiederQuoted 3 lines · Jonathan Nieder
I don't *think* you intend to say "sure, you got user reports, but (those users are wrong | those users are not real | you are not interpreting those users correctly)", but that is what I am hearing.
What I have been saying is "we are sending a wrong message to those users by not clearly saying 'optional' (i.e. it is OK for your Git not to understand this optional bits of information---you do not have to get alarmed immediately) and also not hinting where that optional thing comes from (i.e. if users realized they come from the future, the coalmine canary message will serve its purpose of reminding them that a newer Git is not just available but has been used already in their repository and help them to rectify the situation sooner)".
As the deployed versions of Git will keep sending the wrong message, I do not mind applying 1/5 and 2/5, given especially that Ben seems to be OK with the plan. I however do not think 3 thru 5 is ready yet with this round---there were some discussions on phrasing in this thread.
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Quoted 16 lines · Junio C Hamano
Quoted 3 lines · Jonathan Nieder
I don't *think* you intend to say "sure, you got user reports, but (those users are wrong | those users are not real | you are not interpreting those users correctly)", but that is what I am hearing.
What I have been saying is "we are sending a wrong message to those users by not clearly saying 'optional' (i.e. it is OK for your Git not to understand this optional bits of information---you do not have to get alarmed immediately) and also not hinting where that optional thing comes from (i.e. if users realized they come from the future, the coalmine canary message will serve its purpose of reminding them that a newer Git is not just available but has been used already in their repository and help them to rectify the situation sooner)".
Quoted 5 lines
As the deployed versions of Git will keep sending the wrong message, I do not mind applying 1/5 and 2/5, given especially that Ben seems to be OK with the plan. I however do not think 3 thru 5 is ready yet with this round---there were some discussions on phrasing in this thread.
Thanks much --- that helps a lot.
Would you mind taking patch 4/5 as well? (It's a tweak to the configuration introduced in patches 1 and 2 that addresses a concern Ben Peart had.)
As for patches 3 and 5, I agree. In particular, patch 5 needs an s/performance//, and it seems that the commit messages need some work as well.
Sorry for getting the conversation in the wrong direction, and I'm glad to hear we have a good way forward.
Sincerely,
Jonathan
Re: [PATCH 5/5] index: offer advice for unknown index extensions
In reply to Junio C HamanoQuoted 5 lines · Junio C Hamano
As the deployed versions of Git will keep sending the wrong message, I do not mind applying 1/5 and 2/5, given especially that Ben seems to be OK with the plan. I however do not think 3 thru 5 is ready yet with this round---there were some discussions on phrasing in this thread.
I ran out of time looking at the surrounding code, but I think 1, 2 and 4 form a set that would give us immediate benefit to fast track to the upcoming release.
I do not know if it makes sense to have 3 and 5 separate; I suspect a single patch that does "clarify the warning, and allow those who have no choice in which version of Git to choose squelch it" would suffice.
The phrasing in 5 received a couple of good concrete suggestions already, so it is not ready in its current form but need a bit of wordsmithing. I also do not think a new "trace_printf" would particularly help. If I stared it a lot longer, I may spot more issues in it.
But what that step does primarily would help long after the upcoming release and in that sense can wait a bit longer than 1, 2 & 4 (which I am hoping can be merged in -rc1).
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Quoted 4 lines · Junio C Hamano
I do not know if it makes sense to have 3 and 5 separate; I suspect a single patch that does "clarify the warning, and allow those who have no choice in which version of Git to choose squelch it" would suffice.
I actually do not mind two patches for these, but I think the separation presented in the series is wrong (first to kill the canary completely, and then add it as if it were a completely separate advice).
It would make more sense, at least to me, if
- the earlier step is to clarify the warning on two points (i.e. this is safe to ignore, but you may want to know that you are using a stale Git when we see evidence that a newer one has already been used here) and then
- the later step is to optionally make it possible to squelch it for those who do not have control over what version of Git they are allowed to run.
But again, a single patch to do all of that is also fine.
Re: [PATCH 5/5] index: offer advice for unknown index extensions
In reply to Junio C HamanoQuoted 3 lines · Junio C Hamano
This series has a strong smell of pushing back by the
toolsmiths who refuse to promptly upgrade to help their users, and that is why I do not feel entirely happy with this series.
Last reply, I promise. :)
This sentence might have the key to the misunderstanding. Let me say a little more about where this showed up in the internal deployment here, to clarify things a little.
At Google we deploy snapshots of the "next" branch approximately weekly so that we can find problems early before they affect a published release. We rely on the ability to roll back quickly when a problem is discovered, and we might care more about compatibility than some others because of that.
A popular tool within Google has a bundled copy of Git (also a snapshot of the "next" branch, but from a few weeks prior) and when we deployed Git with the EOIE and IEOT extensions, users of that tool very quickly reported the mysterious message.
That said, the maintainers of that tool did not complain at all, so hopefully I can allay your worries about toolsmiths pushing back. Once the problem reached my attention (a few days later than I would have liked it to), the Git team at Google knew that we could not roll back and were certainly alarmed about what that means about our ability to cope with other problems should we need to. But we were able to quickly update that popular tool --- no issue.
Instead, we ran into a number of other users running into the same problem, when sharing repositories between machines using sshfs, etc. That, plus the aforementioned inability to roll back Git if we need to, meant that this was a serious issue so we quickly addressed it in the internal installation.
In general, we haven't had much trouble getting people to use Git 2.19.1 or newer. So the problem here does not have to do with users being slow to upgrade.
Instead, it's simply that upgrading Git should not cause the older, widely deployed version of Git to complain about the repositories it acts on. That's a recipe for difficult debugging situations, it can lead to people upgrading less quickly and reporting bugs later, and all in all it's a bad situation to be in. I've used tools like Subversion that would upgrade repositories so they are unusable by the previous version and experienced all of these problems.
So I consider it important *to Git upstream* to handle this well in the Git 2.20 release. We can flip the default soon after, even as soon as 2.21.
Moreover, I am not the only one who ran into this --- e.g. from [1], 2018-10-19:
17:10 <peff> jrnieder: Yes, I noticed that annoyance myself. ;) 17:11 <newren> Yeah, I saw that message a few times and was slightly
annoyed as well.
Now, a meta point. Throughout this discussion, I have been hoping for some acknowledgement of the problem --- e.g. an "I am sympathetic to what you are trying to do, but <X>". I wasn't able to find that, and that is part of what contributed to the feeling of not being heard.
Thanks for your patient explanations, and hope that helps, Jonathan
[1] https://colabti.org/irclogger/irclogger_log/git-devel?date=2018-10-19#l114
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Quoted 4 lines · Jonathan Nieder
Now, a meta point. Throughout this discussion, I have been hoping for some acknowledgement of the problem --- e.g. an "I am sympathetic to what you are trying to do, but <X>". I wasn't able to find that, and that is part of what contributed to the feeling of not being heard.
I had little sympathy for what you were trying to do, i.e. killing the coalmine canary that warns users about using older version of Git when there clearly is a sign that a newer one is available to them and they have already used it; there was no problem to be acknowledged.
Not before "why is it different this time?" question gets answered anyway.
And seeing the same "let's not enable the new extension" patch again without much improved justification contributed greatly to the feeling of not being heard. The feeling is mutual.
Re: [PATCH 5/5] index: offer advice for unknown index extensions
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Jonathan Nieder
Now, a meta point. Throughout this discussion, I have been hoping for some acknowledgement of the problem --- e.g. an "I am sympathetic to what you are trying to do, but <X>". I wasn't able to find that, and that is part of what contributed to the feeling of not being heard.
[...]
Quoted 3 lines
And seeing the same "let's not enable the new extension" patch again without much improved justification contributed greatly to the feeling of not being heard.
Thanks for the pointer. I had not understood before that you were unhappy with those commit messages.
The commit message describes symptoms and the motivation for the change. I was confused at the original replies to patch 1 and 2 that seemed to be more about patch 3; patch 1 and 2 are meaningful without patch 3, so it would be odd to include a justification for patch 3 in their commit message.
That said, it sounds like their commit messages are not adequate. I'd appreciate help from someone else to improve them.
Quoted 1 line
The feeling is mutual.
I was trying to diagnose what was going wrong with the conversation so as to move things forward on a better footing. It seems I only escalated things more. :(
Sorry about that, and I hope there's some way to move forward.
What is the best way to handle this? I am feeling somewhat burnt by this review process. If Ben and I, working together, are able to come up with a series that we both like, will you consider it for 2.20? Is there some other trusted contributor, such as Peff or Duy, that you would trust to represent your wishes so I can pursue their Reviewed-by without risking getting burnt in the same way again?
Jonathan
Re: [PATCH 5/5] index: offer advice for unknown index extensions
In reply to Jonathan NiederQuoted 60 lines · Jonathan Nieder
Quoted 3 lines · Junio C Hamano
This series has a strong smell of pushing back by the
toolsmiths who refuse to promptly upgrade to help their users, and that is why I do not feel entirely happy with this series.Last reply, I promise. :)
This sentence might have the key to the misunderstanding. Let me say a little more about where this showed up in the internal deployment here, to clarify things a little.
At Google we deploy snapshots of the "next" branch approximately weekly so that we can find problems early before they affect a published release. We rely on the ability to roll back quickly when a problem is discovered, and we might care more about compatibility than some others because of that.
A popular tool within Google has a bundled copy of Git (also a snapshot of the "next" branch, but from a few weeks prior) and when we deployed Git with the EOIE and IEOT extensions, users of that tool very quickly reported the mysterious message.
That said, the maintainers of that tool did not complain at all, so hopefully I can allay your worries about toolsmiths pushing back. Once the problem reached my attention (a few days later than I would have liked it to), the Git team at Google knew that we could not roll back and were certainly alarmed about what that means about our ability to cope with other problems should we need to. But we were able to quickly update that popular tool --- no issue.
Instead, we ran into a number of other users running into the same problem, when sharing repositories between machines using sshfs, etc. That, plus the aforementioned inability to roll back Git if we need to, meant that this was a serious issue so we quickly addressed it in the internal installation.
In general, we haven't had much trouble getting people to use Git 2.19.1 or newer. So the problem here does not have to do with users being slow to upgrade.
Instead, it's simply that upgrading Git should not cause the older, widely deployed version of Git to complain about the repositories it acts on. That's a recipe for difficult debugging situations, it can lead to people upgrading less quickly and reporting bugs later, and all in all it's a bad situation to be in. I've used tools like Subversion that would upgrade repositories so they are unusable by the previous version and experienced all of these problems.
So I consider it important *to Git upstream* to handle this well in the Git 2.20 release. We can flip the default soon after, even as soon as 2.21.
Moreover, I am not the only one who ran into this --- e.g. from [1], 2018-10-19:
17:10 <peff> jrnieder: Yes, I noticed that annoyance myself. ;) 17:11 <newren> Yeah, I saw that message a few times and was slightly
annoyed as well.
Quoted 7 lines
Now, a meta point. Throughout this discussion, I have been hoping for some acknowledgement of the problem --- e.g. an "I am sympathetic to what you are trying to do, but <X>". I wasn't able to find that, and that is part of what contributed to the feeling of not being heard.
Thanks for your patient explanations, and hope that helps, Jonathan
I think it makes total sense to fix this. I had not spotted this myself since I tend to just roll forward and only use one version of git on one system, but fixing this makes sense.
PATCH 3/3 index: do not warn about unrecognized extensions
In reply to Jonathan NiederDocumentation/technical/index-format explains:
4-byte extension signature. If the first byte is 'A'..'Z' the extension is optional and can be ignored.
This allows gracefully introducing a new index extension without having to rely on all readers having support for it. Mandatory extensions start with a lowercase letter and optional ones start with a capital. Thus the versions of Git acting on a shared local repository do not have to upgrade in lockstep.
We almost obey that convention, but there is a problem: when encountering an unrecognized optional extension, we write
ignoring FNCY extension
to stderr, which alarms users. This means that in practice we have had to introduce index extensions in two steps: first add read support, and then a while later, start writing by default. This delays when users can benefit from improvements to the index format.
We cannot change the past, but for index extensions of the future, there is a straightforward improvement: silence that message except when tracing. This way, the message is still available when debugging, but in everyday use it does not show up so (once most Git users have this patch) we can turn on new optional extensions right away without alarming people.
---
Thanks for reading. Thoughts?
Sincerely,
Jonathan
read-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
read-cache.c +1 -1
@@ -1720,7 +1720,7 @@ static int read_index_extension(struct index_state *istate,if (*ext <'A' ||'Z' < *ext)return error ("index uses %.4s extension, which we do not understand" , ext); -fprintf (stderr,"ignoring %.4s extension\n" , ext); +trace_printf ("ignoring %.4s extension\n" , ext);break ; }return 0 ;
Signature · 1 line
2.19.1.930.g4563a0d9d0
Re: [PATCH 3/3] index: do not warn about unrecognized extensions
Quoted 6 lines · Jonathan Nieder
We almost obey that convention, but there is a problem: when encountering an unrecognized optional extension, we write
ignoring FNCY extension
to stderr, which alarms users.
Then the same comment as 2/3 applies to this step.
Re: [PATCH 3/3] index: do not warn about unrecognized extensions
In reply to Jonathan NiederQuoted 21 lines · Jonathan Nieder
Documentation/technical/index-formatexplains:4-byte extension signature. If the first byte is 'A'..'Z' the extension is optional and can be ignored.
This allows gracefully introducing a new index extension without having to rely on all readers having support for it. Mandatory extensions start with a lowercase letter and optional ones start with a capital. Thus the versions of Git acting on a shared local repository do not have to upgrade in lockstep.
We almost obey that convention, but there is a problem: when encountering an unrecognized optional extension, we write
ignoring FNCY extension
to stderr, which alarms users. This means that in practice we have had to introduce index extensions in two steps: first add read support, and then a while later, start writing by default. This delays when users can benefit from improvements to the index format.
Quoted 7 lines
We cannot change the past, but for index extensions of the future, there is a straightforward improvement: silence that message except when tracing. This way, the message is still available when debugging, but in everyday use it does not show up so (once most Git users have this patch) we can turn on new optional extensions right away without alarming people.
The best patch of the bunch. Glad to see it.
I'm fine with doing this via advise.unknownIndexExtension as well. Who knows, someone may actually want to see this and not have tracing turned on. I don't know who but it is possible :-)
Quoted 24 lines
Signed-off-by Jonathan Nieder <[email protected]>---
Thanks for reading. Thoughts?Sincerely,
Jonathanread-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)read-cache.c +1
-1@@ -1720,7 +1720,7 @@ static int read_index_extension(struct index_state *istate,if (*ext <'A' ||'Z' < *ext)return error ("index uses %.4s extension, which we do not understand" , ext); -fprintf (stderr,"ignoring %.4s extension\n" , ext); +trace_printf ("ignoring %.4s extension\n" , ext);break ; }return 0 ;
Re: [PATCH 3/3] index: do not warn about unrecognized extensions
In reply to Jonathan NiederQuoted 6 lines · Jonathan Nieder
We cannot change the past, but for index extensions of the future, there is a straightforward improvement: silence that message except when tracing. This way, the message is still available when debugging, but in everyday use it does not show up so (once most Git users have this patch) we can turn on new optional extensions right away without alarming people.
That argument ignores the "let the users know they are using a stale version when they did use (either by accident or deliberately) a more recent one" value, though.
Even if we consider that this is only for debugging, I am not sure if tracing is the right place to add. As long as the "optional extensions can be ignored without affecting the correctness" rule holds, there is nothing gained by letting these messages shown for debugging purposes, and if there is such a bug (e.g. we introduced an optional extension but the code that wrote an index with an optional extension wrote the non-optional part in such a way that it cannot be correctly handled without the extension that is supposed to be optional) we'd probably want to let users notice without having to explicitly go into a debugging session. If Googling for "ignoring FNCY ext" gives "discard your index with 'reset HEAD', because an index file with FNCY ext cannot be read without understanding it", that may prevent damages from happening in the first place. On the other hand, hiding it behind tracing would mean the user first need to exprience an unknown breakage first and then has to enable tracing among other 47 different things to diagnose and drill down to the root cause.
Re: [PATCH 3/3] index: do not warn about unrecognized extensions
Quoted 13 lines · Junio C Hamano
Quoted 6 lines · Jonathan Nieder
We cannot change the past, but for index extensions of the future, there is a straightforward improvement: silence that message except when tracing. This way, the message is still available when debugging, but in everyday use it does not show up so (once most Git users have this patch) we can turn on new optional extensions right away without alarming people.
That argument ignores the "let the users know they are using a stale version when they did use (either by accident or deliberately) a more recent one" value, though.
Quoted 5 lines
Even if we consider that this is only for debugging, I am not sure if tracing is the right place to add. As long as the "optional extensions can be ignored without affecting the correctness" rule holds, there is nothing gained by letting these messages shown for debugging purposes
Having recently written a couple of patches that utilize an optional extension - I actually found the warning to be a helpful debugging tool and would like to see them enabled via tracing. It would also be helpful to see the opposite - I'm looking for an optional extension but it is missing.
The most common scenario was when I'd be testing my changes in different repos and forget that I needed to force an updated index to be written that contained the extension I was trying to test. The "silently ignore the optional extension" behavior is good for end users but as a developer, I'd like to be able to have it yell at me via tracing. :-)
IMHO - if an end user has to turn on tracing, I view that as a failure on our part. No end user should have to understand the inner workings of git to be able to use it effectively.
and if there is such a bug (e.g. we introduced
Quoted 14 lines
an optional extension but the code that wrote an index with an optional extension wrote the non-optional part in such a way that it cannot be correctly handled without the extension that is supposed to be optional) we'd probably want to let users notice without having to explicitly go into a debugging session. If Googling for "ignoring FNCY ext" gives "discard your index with 'reset HEAD', because an index file with FNCY ext cannot be read without understanding it", that may prevent damages from happening in the first place. On the other hand, hiding it behind tracing would mean the user first need to exprience an unknown breakage first and then has to enable tracing among other 47 different things to diagnose and drill down to the root cause.