| Message ID | 667ad0dea70cb7f0bbf8f52467f15129b3ae1325.1600814153.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6d996dd20625ca1d0fb0dd815c1f36ea5e942301 |
| Headers | show |
| Series | Parallel Checkout (part I) | expand |
On 9/22/20 6:49 PM, Matheus Tavares wrote: > In a following patch, checkout_entry() will use conv_attrs to decide > whether an entry should be enqueued for parallel checkout or not. But > the attributes lookup only happens lower in this call stack. To avoid > the unnecessary work of loading the attributes twice, let's move it up > to checkout_entry(), and pass the loaded struct down to write_entry(). > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > --- > entry.c | 38 +++++++++++++++++++++++++++----------- > 1 file changed, 27 insertions(+), 11 deletions(-) > > diff --git a/entry.c b/entry.c > index 1d2df188e5..8237859b12 100644 > --- a/entry.c > +++ b/entry.c > @@ -263,8 +263,9 @@ void update_ce_after_write(const struct checkout *state, struct cache_entry *ce, > } > } > > -static int write_entry(struct cache_entry *ce, > - char *path, const struct checkout *state, int to_tempfile) > +/* Note: ca is used (and required) iff the entry refers to a regular file. */ > +static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca, > + const struct checkout *state, int to_tempfile) > { > unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT; > struct delayed_checkout *dco = state->delayed_checkout; > @@ -281,8 +282,7 @@ static int write_entry(struct cache_entry *ce, > clone_checkout_metadata(&meta, &state->meta, &ce->oid); > > if (ce_mode_s_ifmt == S_IFREG) { > - struct stream_filter *filter = get_stream_filter(state->istate, ce->name, > - &ce->oid); > + struct stream_filter *filter = get_stream_filter_ca(ca, &ce->oid); > if (filter && > !streaming_write_entry(ce, path, filter, > state, to_tempfile, > @@ -329,14 +329,17 @@ static int write_entry(struct cache_entry *ce, > * Convert from git internal format to working tree format > */ > if (dco && dco->state != CE_NO_DELAY) { > - ret = async_convert_to_working_tree(state->istate, ce->name, new_blob, > - size, &buf, &meta, dco); > + ret = async_convert_to_working_tree_ca(ca, ce->name, > + new_blob, size, > + &buf, &meta, dco); > if (ret && string_list_has_string(&dco->paths, ce->name)) { > free(new_blob); > goto delayed; > } > - } else > - ret = convert_to_working_tree(state->istate, ce->name, new_blob, size, &buf, &meta); > + } else { > + ret = convert_to_working_tree_ca(ca, ce->name, new_blob, > + size, &buf, &meta); > + } > > if (ret) { > free(new_blob); > @@ -442,6 +445,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, > { > static struct strbuf path = STRBUF_INIT; > struct stat st; > + struct conv_attrs ca; I have to wonder if it would be clearer to move this declaration of `ca` into the two `if { ... }` blocks where it is used -- to indicate that it is only defined in two cases where we call `convert_attrs()`. There are several other calls to `write_entry()` that pass NULL and it could cause confusion. > > if (ce->ce_flags & CE_WT_REMOVE) { > if (topath) > @@ -454,8 +458,13 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, > return 0; > } > > - if (topath) > - return write_entry(ce, topath, state, 1); > + if (topath) { > + if (S_ISREG(ce->ce_mode)) { > + convert_attrs(state->istate, &ca, ce->name); > + return write_entry(ce, topath, &ca, state, 1); > + } > + return write_entry(ce, topath, NULL, state, 1); > + } > > strbuf_reset(&path); > strbuf_add(&path, state->base_dir, state->base_dir_len); > @@ -517,9 +526,16 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, > return 0; > > create_directories(path.buf, path.len, state); > + > if (nr_checkouts) > (*nr_checkouts)++; > - return write_entry(ce, path.buf, state, 0); > + > + if (S_ISREG(ce->ce_mode)) { > + convert_attrs(state->istate, &ca, ce->name); > + return write_entry(ce, path.buf, &ca, state, 0); > + } > + > + return write_entry(ce, path.buf, NULL, state, 0); > } > > void unlink_entry(const struct cache_entry *ce) >
On 10/1/20 11:53 AM, Jeff Hostetler wrote: > > > On 9/22/20 6:49 PM, Matheus Tavares wrote: >> In a following patch, checkout_entry() will use conv_attrs to decide >> whether an entry should be enqueued for parallel checkout or not. But >> the attributes lookup only happens lower in this call stack. To avoid >> the unnecessary work of loading the attributes twice, let's move it up >> to checkout_entry(), and pass the loaded struct down to write_entry(). >> >> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> >> --- >> entry.c | 38 +++++++++++++++++++++++++++----------- >> 1 file changed, 27 insertions(+), 11 deletions(-) >> >> diff --git a/entry.c b/entry.c >> index 1d2df188e5..8237859b12 100644 >> --- a/entry.c >> +++ b/entry.c >> @@ -263,8 +263,9 @@ void update_ce_after_write(const struct checkout >> *state, struct cache_entry *ce, >> } >> } >> -static int write_entry(struct cache_entry *ce, >> - char *path, const struct checkout *state, int >> to_tempfile) >> +/* Note: ca is used (and required) iff the entry refers to a regular >> file. */ >> +static int write_entry(struct cache_entry *ce, char *path, struct >> conv_attrs *ca, >> + const struct checkout *state, int to_tempfile) >> { >> unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT; >> struct delayed_checkout *dco = state->delayed_checkout; >> @@ -281,8 +282,7 @@ static int write_entry(struct cache_entry *ce, >> clone_checkout_metadata(&meta, &state->meta, &ce->oid); >> if (ce_mode_s_ifmt == S_IFREG) { >> - struct stream_filter *filter = >> get_stream_filter(state->istate, ce->name, >> - &ce->oid); >> + struct stream_filter *filter = get_stream_filter_ca(ca, >> &ce->oid); >> if (filter && >> !streaming_write_entry(ce, path, filter, >> state, to_tempfile, >> @@ -329,14 +329,17 @@ static int write_entry(struct cache_entry *ce, >> * Convert from git internal format to working tree format >> */ >> if (dco && dco->state != CE_NO_DELAY) { >> - ret = async_convert_to_working_tree(state->istate, >> ce->name, new_blob, >> - size, &buf, &meta, dco); >> + ret = async_convert_to_working_tree_ca(ca, ce->name, >> + new_blob, size, >> + &buf, &meta, dco); >> if (ret && string_list_has_string(&dco->paths, ce->name)) { >> free(new_blob); >> goto delayed; >> } >> - } else >> - ret = convert_to_working_tree(state->istate, ce->name, >> new_blob, size, &buf, &meta); >> + } else { >> + ret = convert_to_working_tree_ca(ca, ce->name, new_blob, >> + size, &buf, &meta); >> + } >> if (ret) { >> free(new_blob); >> @@ -442,6 +445,7 @@ int checkout_entry(struct cache_entry *ce, const >> struct checkout *state, >> { >> static struct strbuf path = STRBUF_INIT; >> struct stat st; >> + struct conv_attrs ca; > > I have to wonder if it would be clearer to move this declaration of `ca` > into the two `if { ... }` blocks where it is used -- to indicate that it > is only defined in two cases where we call `convert_attrs()`. > > There are several other calls to `write_entry()` that pass NULL and it > could cause confusion. Nevermind, I see what you did in step 9 and this makes sense. > > >> if (ce->ce_flags & CE_WT_REMOVE) { >> if (topath) >> @@ -454,8 +458,13 @@ int checkout_entry(struct cache_entry *ce, const >> struct checkout *state, >> return 0; >> } >> - if (topath) >> - return write_entry(ce, topath, state, 1); >> + if (topath) { >> + if (S_ISREG(ce->ce_mode)) { >> + convert_attrs(state->istate, &ca, ce->name); >> + return write_entry(ce, topath, &ca, state, 1); >> + } >> + return write_entry(ce, topath, NULL, state, 1); >> + } >> strbuf_reset(&path); >> strbuf_add(&path, state->base_dir, state->base_dir_len); >> @@ -517,9 +526,16 @@ int checkout_entry(struct cache_entry *ce, const >> struct checkout *state, >> return 0; >> create_directories(path.buf, path.len, state); >> + >> if (nr_checkouts) >> (*nr_checkouts)++; >> - return write_entry(ce, path.buf, state, 0); >> + >> + if (S_ISREG(ce->ce_mode)) { >> + convert_attrs(state->istate, &ca, ce->name); >> + return write_entry(ce, path.buf, &ca, state, 0); >> + } >> + >> + return write_entry(ce, path.buf, NULL, state, 0); >> } >> void unlink_entry(const struct cache_entry *ce) >>
diff --git a/entry.c b/entry.c index 1d2df188e5..8237859b12 100644 --- a/entry.c +++ b/entry.c @@ -263,8 +263,9 @@ void update_ce_after_write(const struct checkout *state, struct cache_entry *ce, } } -static int write_entry(struct cache_entry *ce, - char *path, const struct checkout *state, int to_tempfile) +/* Note: ca is used (and required) iff the entry refers to a regular file. */ +static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca, + const struct checkout *state, int to_tempfile) { unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT; struct delayed_checkout *dco = state->delayed_checkout; @@ -281,8 +282,7 @@ static int write_entry(struct cache_entry *ce, clone_checkout_metadata(&meta, &state->meta, &ce->oid); if (ce_mode_s_ifmt == S_IFREG) { - struct stream_filter *filter = get_stream_filter(state->istate, ce->name, - &ce->oid); + struct stream_filter *filter = get_stream_filter_ca(ca, &ce->oid); if (filter && !streaming_write_entry(ce, path, filter, state, to_tempfile, @@ -329,14 +329,17 @@ static int write_entry(struct cache_entry *ce, * Convert from git internal format to working tree format */ if (dco && dco->state != CE_NO_DELAY) { - ret = async_convert_to_working_tree(state->istate, ce->name, new_blob, - size, &buf, &meta, dco); + ret = async_convert_to_working_tree_ca(ca, ce->name, + new_blob, size, + &buf, &meta, dco); if (ret && string_list_has_string(&dco->paths, ce->name)) { free(new_blob); goto delayed; } - } else - ret = convert_to_working_tree(state->istate, ce->name, new_blob, size, &buf, &meta); + } else { + ret = convert_to_working_tree_ca(ca, ce->name, new_blob, + size, &buf, &meta); + } if (ret) { free(new_blob); @@ -442,6 +445,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, { static struct strbuf path = STRBUF_INIT; struct stat st; + struct conv_attrs ca; if (ce->ce_flags & CE_WT_REMOVE) { if (topath) @@ -454,8 +458,13 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, return 0; } - if (topath) - return write_entry(ce, topath, state, 1); + if (topath) { + if (S_ISREG(ce->ce_mode)) { + convert_attrs(state->istate, &ca, ce->name); + return write_entry(ce, topath, &ca, state, 1); + } + return write_entry(ce, topath, NULL, state, 1); + } strbuf_reset(&path); strbuf_add(&path, state->base_dir, state->base_dir_len); @@ -517,9 +526,16 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, return 0; create_directories(path.buf, path.len, state); + if (nr_checkouts) (*nr_checkouts)++; - return write_entry(ce, path.buf, state, 0); + + if (S_ISREG(ce->ce_mode)) { + convert_attrs(state->istate, &ca, ce->name); + return write_entry(ce, path.buf, &ca, state, 0); + } + + return write_entry(ce, path.buf, NULL, state, 0); } void unlink_entry(const struct cache_entry *ce)
In a following patch, checkout_entry() will use conv_attrs to decide whether an entry should be enqueued for parallel checkout or not. But the attributes lookup only happens lower in this call stack. To avoid the unnecessary work of loading the attributes twice, let's move it up to checkout_entry(), and pass the loaded struct down to write_entry(). Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- entry.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-)