Re: What's cooking in git.git (Sep 2016, #03; Fri, 9)
On Mon, Sep 12, 2016 at 12:10:13PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > I happened to notice today that this topic needs a minor tweak: > > > > -- >8 -- > > Subject: [PATCH] add_delta_base_cache: use list_for_each_safe > > > > We may remove elements from the list while we are iterating, > > which requires using a second temporary pointer. Otherwise > > stepping to the next element of the list might involve > > looking at freed memory (which generally works in practice, > > as we _just_ freed it, but of course is wrong to rely on; > > valgrind notices it). > > I failed to notice it, too. Thanks. After staring at this, I was wondering how the _original_ ever worked. Because the problem is in the linked-list code, which I did not really change (I switched it to LIST_HEAD(), but the code is equivalent). The answer is that in the original, there was no free() in the original code when we released an entry; it just went back to the (static) pool. So the bug is in the conversion to hashmap, where we start allocating (and freeing) the entries individually. -Peff
Re: What's cooking in git.git (Sep 2016, #03; Fri, 9)
Jeff King writes:
> I happened to notice today that this topic needs a minor tweak:
>
> -- >8 --
> Subject: [PATCH] add_delta_base_cache: use list_for_each_safe
>
> We may remove elements from the list while we are iterating,
> which requires using a second temporary pointer. Otherwise
> stepping to the next element of the list might involve
> looking at freed memory (which generally works in practice,
> as we _just_ freed it, but of course is wrong to rely on;
> valgrind notices it).
I failed to notice it, too. Thanks.
> Signed-off-by: Jeff King
> ---
> sha1_file.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index a57b71d..132c861 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2187,11 +2187,11 @@ static void add_delta_base_cache(struct packed_git
> *p, off_t base_offset,
> void *base, unsigned long base_size, enum object_type type)
> {
> struct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));
> - struct list_head *lru;
> + struct list_head *lru, *tmp;
>
> delta_base_cached += base_size;
>
> - list_for_each(lru, &delta_base_cache_lru) {
> + list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
> struct delta_base_cache_entry *f =
> list_entry(lru, struct delta_base_cache_entry, lru);
> if (delta_base_cached <= delta_base_cache_limit)
Re: What's cooking in git.git (Sep 2016, #03; Fri, 9)
On Fri, Sep 09, 2016 at 03:47:35PM -0700, Junio C Hamano wrote:
> * jk/delta-base-cache (2016-08-23) 7 commits
> (merged to 'next' on 2016-08-25 at f1c141a)
> + t/perf: add basic perf tests for delta base cache
> + delta_base_cache: use hashmap.h
> + delta_base_cache: drop special treatment of blobs
> + delta_base_cache: use list.h for LRU
> + release_delta_base_cache: reuse existing detach function
> + clear_delta_base_cache_entry: use a more descriptive name
> + cache_or_unpack_entry: drop keep_cache parameter
>
> The delta-base-cache mechanism has been a key to the performance in
> a repository with a tightly packed packfile, but it did not scale
> well even with a larger value of core.deltaBaseCacheLimit.
I happened to notice today that this topic needs a minor tweak:
-- >8 --
Subject: [PATCH] add_delta_base_cache: use list_for_each_safe
We may remove elements from the list while we are iterating,
which requires using a second temporary pointer. Otherwise
stepping to the next element of the list might involve
looking at freed memory (which generally works in practice,
as we _just_ freed it, but of course is wrong to rely on;
valgrind notices it).
Signed-off-by: Jeff King
---
sha1_file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index a57b71d..132c861 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2187,11 +2187,11 @@ static void add_delta_base_cache(struct packed_git *p,
off_t base_offset,
void *base, unsigned long base_size, enum object_type type)
{
struct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));
- struct list_head *lru;
+ struct list_head *lru, *tmp;
delta_base_cached += base_size;
- list_for_each(lru, &delta_base_cache_lru) {
+ list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
struct delta_base_cache_entry *f =
list_entry(lru, struct delta_base_cache_entry, lru);
if (delta_base_cached <= delta_base_cache_limit)
--
2.10.0.230.g6f8d04b
What's cooking in git.git (Sep 2016, #03; Fri, 9)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. I've started merging topics that have been cooking in 'next' to 'master', and the next step will be to rewind and rebuild 'next', and merge those that have been waiting in 'pu' to 'next'. There are a few more topics in flight that may be ready to be picked up but I haven't, and other topics in flight that may not be quite ready. They will be picked up after topics that have already been in-tree starts quieting down. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * bh/diff-highlight-graph (2016-08-31) 6 commits (merged to 'next' on 2016-08-31 at 523a15f) + diff-highlight: avoid highlighting combined diffs + diff-highlight: add multi-byte tests + diff-highlight: ignore test cruft + diff-highlight: add support for --graph output + diff-highlight: add failing test for handling --graph output + diff-highlight: add some tests "diff-highlight" script (in contrib/) learned to work better with "git log -p --graph" output. * cc/receive-pack-limit (2016-08-24) 3 commits (merged to 'next' on 2016-08-25 at bc74b5b) + receive-pack: allow a maximum input size to be specified + unpack-objects: add --max-input-size= option + index-pack: add --max-input-size= option An incoming "git push" that attempts to push too many bytes can now be rejected by setting a new configuration variable at the receiving end. * hv/doc-commit-reference-style (2016-08-26) 1 commit (merged to 'next' on 2016-08-31 at 68fb778) + SubmittingPatches: use gitk's "Copy commit summary" format A small doc update. * jh/status-v2-porcelain (2016-08-12) 9 commits (merged to 'next' on 2016-08-31 at e71f595) + status: unit tests for --porcelain=v2 + test-lib-functions.sh: add lf_to_nul helper + git-status.txt: describe --porcelain=v2 format + status: print branch info with --porcelain=v2 --branch + status: print per-file porcelain v2 status data + status: collect per-file data for --porcelain=v2 + status: support --porcelain[=] + status: cleanup API to wt_status_print + status: rename long-format print routines Enhance "git status --porcelain" output by collecting more data on the state of the index and the working tree files, which may further be used to teach git-prompt (in contrib/) to make fewer calls to git. * jk/delta-base-cache (2016-08-23) 7 commits (merged to 'next' on 2016-08-25 at f1c141a) + t/perf: add basic perf tests for delta base cache + delta_base_cache: use hashmap.h + delta_base_cache: drop special treatment of blobs + delta_base_cache: use list.h for LRU + release_delta_base_cache: reuse existing detach function + clear_delta_base_cache_entry: use a more descriptive name + cache_or_unpack_entry: drop keep_cache parameter The delta-base-cache mechanism has been a key to the performance in a repository with a tightly packed packfile, but it did not scale well even with a larger value of core.deltaBaseCacheLimit. * jk/format-patch-number-singleton-patch-with-cover (2016-08-23) 1 commit (merged to 'next' on 2016-08-25 at a4737fb) + format-patch: show 0/1 and 1/1 for singleton patch with cover letter "git format-patch --cover-letter HEAD^" to format a single patch with a separate cover letter now numbers the output as [PATCH 0/1] and [PATCH 1/1] by default. * po/range-doc (2016-08-13) 12 commits (merged to 'next' on 2016-08-31 at d29870b) + doc: revisions: sort examples and fix alignment of the unchanged + doc: revisions: show revision expansion in examples + doc: revisions - clarify reachability examples + doc: revisions - define `reachable` + doc: gitrevisions - clarify 'latter case' is revision walk + doc: gitrevisions - use 'reachable' in page description + doc: revisions: single vs multi-parent notation comparison + doc: revisions: extra clarification of ^! notation effects + doc: revisions: give headings for the two and three dot notations + doc: show the actual left, right, and boundary marks + doc: revisions - name the left and right sides + doc: use 'symmetric difference' consistently Clarify various ways to specify the "revision ranges" in the documentation. * rt/help-unknown (2016-08-30) 3 commits (merged to 'next' on 2016-08-30 at db2a5b0) + help: make option --help open man pages only for Git commands + help: introduce option --exclude-guides + Merge branch 'js/no-html-bypass-on-windows' into rt/help-unknown "git nosuchcommand --help" said "No manual entry for gitnosuchcommand", which was not intuitive, given that "git nosuchcommand" said "git: 'nosuchcommand' is not a git command".
