From: Junio C Hamano <hidden> Date: 2017-05-02 01:36:37
Stefan Beller [off-list ref] writes:
This applies to origin/master.
For better readability and understandability for newcomers it is a good idea
to not offer 2 APIs doing the same thing with on being the #define of the other.
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Why? Why should we keep typing &the_index, when most of the time we
are given _the_ index and working on it?
From: Stefan Beller <hidden> Date: 2017-05-02 04:17:51
On Mon, May 1, 2017 at 6:36 PM, Junio C Hamano [off-list ref] wrote:
Stefan Beller [off-list ref] writes:
quoted
This applies to origin/master.
For better readability and understandability for newcomers it is a good idea
to not offer 2 APIs doing the same thing with on being the #define of the other.
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Why? Why should we keep typing &the_index, when most of the time we
are given _the_ index and working on it?
As someone knowledgeable with the code base you know that the cache_*
and index_* functions only differ by an index argument. A newcomer may not
know this, so they wonder why we have (A) so many functions [and which is the
right function to use]; it is an issue of ease of use of the code base.
Anything you do In submodule land today needs to spawn new processes in
the submodule. This is cumbersome and not performant. So in the far future
we may want to have an abstraction of a repo (B), i.e. all repository state in
one struct/class. That way we can open a submodule in-process and perform
the required actions without spawning a process.
The road to (B) is a long road, but we have to art somewhere. And this seemed
like a good place by introducing a dedicated argument for the
repository. In a follow
up in the future we may want to replace &the_index by "the_main_repo.its_index"
and then could also run the commands on other (submodule) indexes. But more
importantly, all these commands would operate on a repository object.
In such a far future we would have functions like the cmd_* functions
that would take a repository object instead of doing its setup discovery
on their own.
Another reason may be its current velocity (or absence of it) w.r.t. to these
functions, such that fewer merge conflicts may arise.
---
This discussion is similar to the "free memory at the end of cmd_*" discussion,
as it aims to make code reusable, and accepting a minor drawback for it.
Typing "the_index" re-enforces the object thinking model and may have people
start on thinking if they would like to declare yet another global variable.
Thanks,
Stefan
From: Jeff Hostetler <hidden> Date: 2017-05-02 14:05:11
On 5/2/2017 12:17 AM, Stefan Beller wrote:
On Mon, May 1, 2017 at 6:36 PM, Junio C Hamano [off-list ref] wrote:
quoted
Stefan Beller [off-list ref] writes:
quoted
This applies to origin/master.
For better readability and understandability for newcomers it is a good idea
to not offer 2 APIs doing the same thing with on being the #define of the other.
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Thank you for bringing this up and making this proposal.
I started a similar effort internally last fall, but
stopped because of the footprint size.
quoted
Why? Why should we keep typing &the_index, when most of the time we
are given _the_ index and working on it?
As someone knowledgeable with the code base you know that the cache_*
and index_* functions only differ by an index argument. A newcomer may not
know this, so they wonder why we have (A) so many functions [and which is the
right function to use]; it is an issue of ease of use of the code base.
Anything you do In submodule land today needs to spawn new processes in
the submodule. This is cumbersome and not performant. So in the far future
we may want to have an abstraction of a repo (B), i.e. all repository state in
one struct/class. That way we can open a submodule in-process and perform
the required actions without spawning a process.
The road to (B) is a long road, but we have to art somewhere. And this seemed
like a good place by introducing a dedicated argument for the
repository. In a follow
up in the future we may want to replace &the_index by "the_main_repo.its_index"
and then could also run the commands on other (submodule) indexes. But more
importantly, all these commands would operate on a repository object.
In such a far future we would have functions like the cmd_* functions
that would take a repository object instead of doing its setup discovery
on their own.
Another reason may be its current velocity (or absence of it) w.r.t. to these
functions, such that fewer merge conflicts may arise.
In addition to (eventually) allowing multiple repos be open at
the same time for submodules, it would also help with various
multi-threading efforts. For example, we have loops that do a
"for (k = 0, k < active_nr; k++) {...}" There is no visual clue
in that code that it references "the_index" and therefore should
be subject to the same locking. Granted, this is a trivial example,
but goes to the argument that the code has lots of subtle global
variables and macros that make it difficult to reason about the
code.
This step would help un-hide this.
In a much longer future, we could also consider building an
improved API around the in-memory index data. For example,
currently we have a simple array of cache_entry pointers and
the entire code base uses "for" loops like the above to iterate.
If we could hide that fact, then we could consider alternative
representations for various reasons.
() bulk alloc the cache_entries from a pool, rather than individually.
() cluster cache_entries linearly by parent directory, rather
than linearly over the whole tree.
() efficient alternative iterator methods on the index, such as
non-recursive breadth-first
Things like this would be difficult with the current set of
globals and macros.
Thanks,
Jeff
---
This discussion is similar to the "free memory at the end of cmd_*" discussion,
as it aims to make code reusable, and accepting a minor drawback for it.
Typing "the_index" re-enforces the object thinking model and may have people
start on thinking if they would like to declare yet another global variable.
Thanks,
Stefan
On Tue, May 2, 2017 at 8:36 AM, Junio C Hamano [off-list ref] wrote:
Stefan Beller [off-list ref] writes:
quoted
This applies to origin/master.
For better readability and understandability for newcomers it is a good idea
to not offer 2 APIs doing the same thing with on being the #define of the other.
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Why?
Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k
Why should we keep typing &the_index, when most of the time we are given _the_ index and working on it?
I attempted the same thing once or twice in the past, had the same
impression and dropped it. But I think it's good to get rid of cache_*
macros, at least outside builtin/ directory. It makes it clearer about
the index dependency. Maybe one day we could libify sha1_file.c and
finally introduce "struct repository", where the_index, object db and
ref-store belong (hmm.. the index may belong to "struct worktree", not
the repo one...).
So yeah it may look a bit more verbose (and probably causes a lot more
conflicts on 'pu') but in my opinion it's a good direction. I wish
Stefan good luck. Brave soul :D
--
Duy
From: Samuel Lijin <hidden> Date: 2017-05-03 11:32:34
On Tue, May 2, 2017 at 9:05 AM, Jeff Hostetler [off-list ref] wrote:
On 5/2/2017 12:17 AM, Stefan Beller wrote:
quoted
On Mon, May 1, 2017 at 6:36 PM, Junio C Hamano [off-list ref] wrote:
quoted
Stefan Beller [off-list ref] writes:
quoted
This applies to origin/master.
For better readability and understandability for newcomers it is a good
idea
to not offer 2 APIs doing the same thing with on being the #define of
the other.
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Thank you for bringing this up and making this proposal.
I started a similar effort internally last fall, but
stopped because of the footprint size.
quoted
quoted
Why? Why should we keep typing &the_index, when most of the time we
are given _the_ index and working on it?
As someone knowledgeable with the code base you know that the cache_*
and index_* functions only differ by an index argument. A newcomer may not
know this, so they wonder why we have (A) so many functions [and which is
the
right function to use]; it is an issue of ease of use of the code base.
Anything you do In submodule land today needs to spawn new processes in
the submodule. This is cumbersome and not performant. So in the far future
we may want to have an abstraction of a repo (B), i.e. all repository
state in
one struct/class. That way we can open a submodule in-process and perform
the required actions without spawning a process.
The road to (B) is a long road, but we have to art somewhere. And this
seemed
like a good place by introducing a dedicated argument for the
repository. In a follow
up in the future we may want to replace &the_index by
"the_main_repo.its_index"
and then could also run the commands on other (submodule) indexes. But
more
importantly, all these commands would operate on a repository object.
In such a far future we would have functions like the cmd_* functions
that would take a repository object instead of doing its setup discovery
on their own.
Another reason may be its current velocity (or absence of it) w.r.t. to
these
functions, such that fewer merge conflicts may arise.
In addition to (eventually) allowing multiple repos be open at
the same time for submodules, it would also help with various
multi-threading efforts. For example, we have loops that do a
"for (k = 0, k < active_nr; k++) {...}" There is no visual clue
in that code that it references "the_index" and therefore should
be subject to the same locking. Granted, this is a trivial example,
but goes to the argument that the code has lots of subtle global
variables and macros that make it difficult to reason about the
code.
Just to throw out an example, I'm relatively new to the codebase (I've
been lurking on the mailing list for a few months now) and for a
recent project (I'm an undergrad wrapping up my senior year, and one
of my classes' final projects was to do something that involved
concurrency) I took a shot at parallelizing the estimate_similarity()
calls in diffcore_rename(). The only way I was able to get it to work
was by dropping global mutexes in one or two files (the code for those
mutexes still makes me cringe), because of concurrent writes to global
data structures.
From: Stefan Beller <hidden> Date: 2017-05-03 17:02:48
On Wed, May 3, 2017 at 3:27 AM, Duy Nguyen [off-list ref] wrote:
On Tue, May 2, 2017 at 8:36 AM, Junio C Hamano [off-list ref] wrote:
quoted
Stefan Beller [off-list ref] writes:
quoted
This applies to origin/master.
For better readability and understandability for newcomers it is a good idea
to not offer 2 APIs doing the same thing with on being the #define of the other.
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Why?
Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k
quoted
Why should we keep typing &the_index, when most of the time we are given _the_ index and working on it?
I attempted the same thing once or twice in the past, had the same
impression and dropped it. But I think it's good to get rid of cache_*
macros, at least outside builtin/ directory. It makes it clearer about
the index dependency. Maybe one day we could libify sha1_file.c and
finally introduce "struct repository", where the_index, object db and
ref-store belong (hmm.. the index may belong to "struct worktree", not
the repo one...).
+cc Brandon, who attempts to come up with a struct repository as we speak.
So yeah it may look a bit more verbose (and probably causes a lot more
conflicts on 'pu') but in my opinion it's a good direction. I wish
Stefan good luck. Brave soul :D
From: Stefan Beller <hidden> Date: 2017-05-03 17:15:01
On Wed, May 3, 2017 at 4:31 AM, Samuel Lijin [off-list ref] wrote:
On Tue, May 2, 2017 at 9:05 AM, Jeff Hostetler [off-list ref] wrote:
quoted
On 5/2/2017 12:17 AM, Stefan Beller wrote:
quoted
On Mon, May 1, 2017 at 6:36 PM, Junio C Hamano [off-list ref] wrote:
quoted
Stefan Beller [off-list ref] writes:
quoted
This applies to origin/master.
For better readability and understandability for newcomers it is a good
idea
to not offer 2 APIs doing the same thing with on being the #define of
the other.
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Thank you for bringing this up and making this proposal.
I started a similar effort internally last fall, but
stopped because of the footprint size.
quoted
quoted
Why? Why should we keep typing &the_index, when most of the time we
are given _the_ index and working on it?
As someone knowledgeable with the code base you know that the cache_*
and index_* functions only differ by an index argument. A newcomer may not
know this, so they wonder why we have (A) so many functions [and which is
the
right function to use]; it is an issue of ease of use of the code base.
Anything you do In submodule land today needs to spawn new processes in
the submodule. This is cumbersome and not performant. So in the far future
we may want to have an abstraction of a repo (B), i.e. all repository
state in
one struct/class. That way we can open a submodule in-process and perform
the required actions without spawning a process.
The road to (B) is a long road, but we have to art somewhere. And this
seemed
like a good place by introducing a dedicated argument for the
repository. In a follow
up in the future we may want to replace &the_index by
"the_main_repo.its_index"
and then could also run the commands on other (submodule) indexes. But
more
importantly, all these commands would operate on a repository object.
In such a far future we would have functions like the cmd_* functions
that would take a repository object instead of doing its setup discovery
on their own.
Another reason may be its current velocity (or absence of it) w.r.t. to
these
functions, such that fewer merge conflicts may arise.
In addition to (eventually) allowing multiple repos be open at
the same time for submodules, it would also help with various
multi-threading efforts. For example, we have loops that do a
"for (k = 0, k < active_nr; k++) {...}" There is no visual clue
in that code that it references "the_index" and therefore should
be subject to the same locking. Granted, this is a trivial example,
but goes to the argument that the code has lots of subtle global
variables and macros that make it difficult to reason about the
code.
Just to throw out an example, I'm relatively new to the codebase (I've
been lurking on the mailing list for a few months now) and for a
recent project (I'm an undergrad wrapping up my senior year, and one
of my classes' final projects was to do something that involved
concurrency) I took a shot at parallelizing the estimate_similarity()
calls in diffcore_rename(). The only way I was able to get it to work
was by dropping global mutexes in one or two files (the code for those
mutexes still makes me cringe), because of concurrent writes to global
data structures.
From: Samuel Lijin <hidden> Date: 2017-05-03 18:23:36
On Wed, May 3, 2017 at 12:14 PM, Stefan Beller [off-list ref] wrote:
On Wed, May 3, 2017 at 4:31 AM, Samuel Lijin [off-list ref] wrote:
quoted
Just to throw out an example, I'm relatively new to the codebase (I've
been lurking on the mailing list for a few months now) and for a
recent project (I'm an undergrad wrapping up my senior year, and one
of my classes' final projects was to do something that involved
concurrency) I took a shot at parallelizing the estimate_similarity()
calls in diffcore_rename(). The only way I was able to get it to work
was by dropping global mutexes in one or two files (the code for those
mutexes still makes me cringe), because of concurrent writes to global
data structures.
I was planning on revisiting the patch series before sending it out -
the changes in attr.c and sha1_file.c are not pretty (and I'm pretty
sure one of them is non-portable) - but it is published at
https://github.com/sxlijin/git/commits/parallelize.thread-pool.1 (it's
based off v2.12.2).
From: Junio C Hamano <hidden> Date: 2017-05-04 02:48:30
Duy Nguyen [off-list ref] writes:
quoted
quoted
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Why?
Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k
That was to mark the code that implements the underlying machinery
that the code in a particular file has already been vetted and
converted to be capable of working with an arbitrary index_state
instance, not just the_index instance.
Your mentioning "at least outside builtin/" shows that you have a
good understanding of the issue; they are infrastructure code and
many of them may become more useful if they are enhanced to take an
index_state instance instead of always working with the_index. With
an infrastructure code that insists on working on the_index, an
application that has something valuable in the_index already needs
to somehow save it elsewhere before calling the function and use the
result from the_index before restoring its version.
I think unpack-trees.c used to assume that it is OK to use the_index,
but after it started taking src/dst_index, it gained NO_THE_INDEX_COMP
thing. That's the kind of change in this area we would want to see.
I attempted the same thing once or twice in the past, had the same
impression and dropped it. But I think it's good to get rid of cache_*
macros, at least outside builtin/ directory.
One thing that needs to be kept in mind is that a mechanical
replacement of active_cache[] to the_index.cache[] in the
infrastructure code does not get us closer to such an improvement.
In fact, such a patch only has negative value (keep reading until
the end of the paragraph that begins with "One thing is certain" to
understand why).
The entry point to a codechain for an important infrastructure code
that can currently only work with the_index needs to be identified.
There may be many. They need to be made to take an index_state
instance and to pass it through the callchain. That kind of change
would be an improvement, and will get us closer to mark the
resulting code with NO_THE_INDEX_COMP thing.
I think somebody else said this, but I agree that in the longer
term, we would want to have a "repository" abstraction for in-core
data structure.
Imagine that a program wants to read into an index_state the tip
commit of a branch whose name is the same as the current branch in
one submodule and do something with the objects, while keeping
the_index for the top-level superproject. Thanks to Michael's and
your recent work, we are almost there to have "ref-store" that
represents the set of refs the submodule in question has that a
"repository" object that represents the submodule can point at. We
can learn the object name at the tip of a specific ref using that
infrastructure. Thanks to an ancient change under discussion, we
can already read the contents of the index file into an instance of
an index_state that is different from the_index. But after doing
these operations, we'd need to actually get to the contents of the
objects the index_state holds. How should that work in the
"repository object represents an in-core repository" world?
What is missing is a way to represent the object store, so that the
"repository" object can point at an instance of it (which in turn
may point at a set of its alternates) [*1*]. With that, perhaps the
most general kind of the infrastructure API that works with an
index_state instance may additionally take a repository object, or
perhaps an index_state may have a pointer to the repository where
the objects listed in it must be taken from (in which case the
function may take only an index_state). In the end the enhanced
function has to allow us to read from the object store of the
submodule, not from the superproject's.
Depending on how the design of the repository object goes, the
interface to these functions may have to change.
But one thing is certain. Many users of the API, just like a lot of
builtin/ code works only with the_index today, will not have to work
with a non-default repository or a non-default index_state. Only
the more advanced and complex "multi-repo" Porcelains would have to
care. Keeping active_cache[], active_nr, etc. abstraction would
isolate the simpler callers from the inevitable code churn we would
need while getting the "repository" abstraction right. This is why
I see that you understood the issues well when you said "builtiln/".
If we want to have the "repository" abstraction, the "object store"
is the next thing we should be working on, and such a work can take
inspiration from how the old <active_cache[], active_nr,
active_alloc, active_cache_changed and *active_cache_tree> tuple was
turned into an "index" object while allowing the majority of code
that want to deal with the singleton instance keep using the old
names via CPP macros.
Also, dropping compatibility macros at the end of the series is
unfriendly to fellow developers with WIPs that depend on the
traditional way of doing things. It is doubly insulting to them to
send such a series during the feature freeze period, when it is more
likely that they are holding off their WIP in an attempt to allow us
to concentrate more on what we should be, i.e. finding and fixing
possible regressions at the tip of 'master' to polish the upcoming
release and help the end users.
[Footnote]
*1* Instead we have an ugly hack that lets the singleton object
store add objects from the submodule object store, and it does not
allow us to back out once we did so (unlike the "ref-store" thing
that we could). When we need to work with a project with many
submodules, we should be able to say "OK, we are done with this
thing for now, call repository_clear() to reclaim the resources we
used to deal with it".
From: Brandon Williams <hidden> Date: 2017-05-04 03:24:23
On 05/03, Junio C Hamano wrote:
Duy Nguyen [off-list ref] writes:
quoted
quoted
quoted
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Why?
Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k
That was to mark the code that implements the underlying machinery
that the code in a particular file has already been vetted and
converted to be capable of working with an arbitrary index_state
instance, not just the_index instance.
Your mentioning "at least outside builtin/" shows that you have a
good understanding of the issue; they are infrastructure code and
many of them may become more useful if they are enhanced to take an
index_state instance instead of always working with the_index. With
an infrastructure code that insists on working on the_index, an
application that has something valuable in the_index already needs
to somehow save it elsewhere before calling the function and use the
result from the_index before restoring its version.
I think unpack-trees.c used to assume that it is OK to use the_index,
but after it started taking src/dst_index, it gained NO_THE_INDEX_COMP
thing. That's the kind of change in this area we would want to see.
quoted
I attempted the same thing once or twice in the past, had the same
impression and dropped it. But I think it's good to get rid of cache_*
macros, at least outside builtin/ directory.
One thing that needs to be kept in mind is that a mechanical
replacement of active_cache[] to the_index.cache[] in the
infrastructure code does not get us closer to such an improvement.
In fact, such a patch only has negative value (keep reading until
the end of the paragraph that begins with "One thing is certain" to
understand why).
The entry point to a codechain for an important infrastructure code
that can currently only work with the_index needs to be identified.
There may be many. They need to be made to take an index_state
instance and to pass it through the callchain. That kind of change
would be an improvement, and will get us closer to mark the
resulting code with NO_THE_INDEX_COMP thing.
I think somebody else said this, but I agree that in the longer
term, we would want to have a "repository" abstraction for in-core
data structure.
Imagine that a program wants to read into an index_state the tip
commit of a branch whose name is the same as the current branch in
one submodule and do something with the objects, while keeping
the_index for the top-level superproject. Thanks to Michael's and
your recent work, we are almost there to have "ref-store" that
represents the set of refs the submodule in question has that a
"repository" object that represents the submodule can point at. We
can learn the object name at the tip of a specific ref using that
infrastructure. Thanks to an ancient change under discussion, we
can already read the contents of the index file into an instance of
an index_state that is different from the_index. But after doing
these operations, we'd need to actually get to the contents of the
objects the index_state holds. How should that work in the
"repository object represents an in-core repository" world?
What is missing is a way to represent the object store, so that the
"repository" object can point at an instance of it (which in turn
may point at a set of its alternates) [*1*]. With that, perhaps the
most general kind of the infrastructure API that works with an
index_state instance may additionally take a repository object, or
perhaps an index_state may have a pointer to the repository where
the objects listed in it must be taken from (in which case the
function may take only an index_state). In the end the enhanced
function has to allow us to read from the object store of the
submodule, not from the superproject's.
Depending on how the design of the repository object goes, the
interface to these functions may have to change.
But one thing is certain. Many users of the API, just like a lot of
builtin/ code works only with the_index today, will not have to work
with a non-default repository or a non-default index_state. Only
the more advanced and complex "multi-repo" Porcelains would have to
care. Keeping active_cache[], active_nr, etc. abstraction would
isolate the simpler callers from the inevitable code churn we would
need while getting the "repository" abstraction right. This is why
I see that you understood the issues well when you said "builtiln/".
If we want to have the "repository" abstraction, the "object store"
is the next thing we should be working on, and such a work can take
inspiration from how the old <active_cache[], active_nr,
active_alloc, active_cache_changed and *active_cache_tree> tuple was
turned into an "index" object while allowing the majority of code
that want to deal with the singleton instance keep using the old
names via CPP macros.
Also, dropping compatibility macros at the end of the series is
unfriendly to fellow developers with WIPs that depend on the
traditional way of doing things. It is doubly insulting to them to
send such a series during the feature freeze period, when it is more
likely that they are holding off their WIP in an attempt to allow us
to concentrate more on what we should be, i.e. finding and fixing
possible regressions at the tip of 'master' to polish the upcoming
release and help the end users.
[Footnote]
*1* Instead we have an ugly hack that lets the singleton object
store add objects from the submodule object store, and it does not
allow us to back out once we did so (unlike the "ref-store" thing
that we could). When we need to work with a project with many
submodules, we should be able to say "OK, we are done with this
thing for now, call repository_clear() to reclaim the resources we
used to deal with it".
Thanks for the detailed explanation on what a possible transition to
having a repository object would look like. While digging through the
code it does seem like the object store is one of the main things
holding the code base back from having a repository object.
Thank being said, I spent today hacking away at 'ls-files' in order to
see if it would be feasible to convert it to using a repository object
for the recursive submodule case instead of using run-command. I picked
'ls-files' mainly because the majority of the code paths don't touch the
object store and mainly deal with manipulating the index. Despite the
hack-y, cobbled together state that it is in currently it seems very
feasible goal.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2017-05-04 03:29:29
On 05/03, Samuel Lijin wrote:
On Wed, May 3, 2017 at 12:14 PM, Stefan Beller [off-list ref] wrote:
quoted
On Wed, May 3, 2017 at 4:31 AM, Samuel Lijin [off-list ref] wrote:
quoted
Just to throw out an example, I'm relatively new to the codebase (I've
been lurking on the mailing list for a few months now) and for a
recent project (I'm an undergrad wrapping up my senior year, and one
of my classes' final projects was to do something that involved
concurrency) I took a shot at parallelizing the estimate_similarity()
calls in diffcore_rename(). The only way I was able to get it to work
was by dropping global mutexes in one or two files (the code for those
mutexes still makes me cringe), because of concurrent writes to global
data structures.
I was planning on revisiting the patch series before sending it out -
the changes in attr.c and sha1_file.c are not pretty (and I'm pretty
sure one of them is non-portable) - but it is published at
https://github.com/sxlijin/git/commits/parallelize.thread-pool.1 (it's
based off v2.12.2).
I spent a good chunk of time reworking the attribute code a few months
back in order to make it thread-safe so you may want to rebase on top of
a newer version and see if your changes get a little less messy (if so
then I did my job :).
--
Brandon Williams
From: Stefan Beller <hidden> Date: 2017-05-04 18:35:51
On Wed, May 3, 2017 at 7:48 PM, Junio C Hamano [off-list ref] wrote:
Duy Nguyen [off-list ref] writes:
quoted
quoted
quoted
In the long run we may want to drop the macros guarded by
NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
Why?
Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k
That was to mark the code that implements the underlying machinery
that the code in a particular file has already been vetted and
converted to be capable of working with an arbitrary index_state
instance, not just the_index instance.
Your mentioning "at least outside builtin/" shows that you have a
good understanding of the issue; they are infrastructure code and
many of them may become more useful if they are enhanced to take an
index_state instance instead of always working with the_index. With
an infrastructure code that insists on working on the_index, an
application that has something valuable in the_index already needs
to somehow save it elsewhere before calling the function and use the
result from the_index before restoring its version.
I think unpack-trees.c used to assume that it is OK to use the_index,
but after it started taking src/dst_index, it gained NO_THE_INDEX_COMP
thing. That's the kind of change in this area we would want to see.
quoted
I attempted the same thing once or twice in the past, had the same
impression and dropped it. But I think it's good to get rid of cache_*
macros, at least outside builtin/ directory.
One thing that needs to be kept in mind is that a mechanical
replacement of active_cache[] to the_index.cache[] in the
infrastructure code does not get us closer to such an improvement.
In fact, such a patch only has negative value (keep reading until
the end of the paragraph that begins with "One thing is certain" to
understand why).
The entry point to a codechain for an important infrastructure code
that can currently only work with the_index needs to be identified.
There may be many. They need to be made to take an index_state
instance and to pass it through the callchain. That kind of change
would be an improvement, and will get us closer to mark the
resulting code with NO_THE_INDEX_COMP thing.
I think somebody else said this, but I agree that in the longer
term, we would want to have a "repository" abstraction for in-core
data structure.
Imagine that a program wants to read into an index_state the tip
commit of a branch whose name is the same as the current branch in
one submodule and do something with the objects, while keeping
the_index for the top-level superproject. Thanks to Michael's and
your recent work, we are almost there to have "ref-store" that
represents the set of refs the submodule in question has that a
"repository" object that represents the submodule can point at. We
can learn the object name at the tip of a specific ref using that
infrastructure. Thanks to an ancient change under discussion, we
can already read the contents of the index file into an instance of
an index_state that is different from the_index. But after doing
these operations, we'd need to actually get to the contents of the
objects the index_state holds. How should that work in the
"repository object represents an in-core repository" world?
What is missing is a way to represent the object store, so that the
"repository" object can point at an instance of it (which in turn
may point at a set of its alternates) [*1*]. With that, perhaps the
most general kind of the infrastructure API that works with an
index_state instance may additionally take a repository object, or
perhaps an index_state may have a pointer to the repository where
the objects listed in it must be taken from (in which case the
function may take only an index_state). In the end the enhanced
function has to allow us to read from the object store of the
submodule, not from the superproject's.
Depending on how the design of the repository object goes, the
interface to these functions may have to change.
But one thing is certain. Many users of the API, just like a lot of
builtin/ code works only with the_index today, will not have to work
with a non-default repository or a non-default index_state. Only
the more advanced and complex "multi-repo" Porcelains would have to
care. Keeping active_cache[], active_nr, etc. abstraction would
isolate the simpler callers from the inevitable code churn we would
need while getting the "repository" abstraction right. This is why
I see that you understood the issues well when you said "builtiln/".
If we want to have the "repository" abstraction, the "object store"
is the next thing we should be working on, and such a work can take
inspiration from how the old <active_cache[], active_nr,
active_alloc, active_cache_changed and *active_cache_tree> tuple was
turned into an "index" object while allowing the majority of code
that want to deal with the singleton instance keep using the old
names via CPP macros.
Thanks for the reply.
So instead of a mechanical replacement, we'd rather want to
see "the_index" not appearing at all outside of builtins, which
implies two things:
* If done properly we can move the macros from cache.h to
e.g. builtin.h. That way future developers are less tempted
to use the cache_* macros in the library code.
* we'd have to pass through the_index from the builtin function
down to the library code, potentially going through multiple
function. For this it is unclear if we want to start this now, or wait
until Brandon presents his initial repository object struct, which
may be suited better for passing-around.
So if I want to further look into refactoring, I'll have a look into
the object store and hold off sending a series that drops the_index.
Also, dropping compatibility macros at the end of the series is
unfriendly to fellow developers with WIPs that depend on the
traditional way of doing things. It is doubly insulting to them to
send such a series during the feature freeze period, when it is more
likely that they are holding off their WIP in an attempt to allow us
to concentrate more on what we should be, i.e. finding and fixing
possible regressions at the tip of 'master' to polish the upcoming
release and help the end users.
Personally I have not noticed large variations of patch volume
correlated to pre-release times.
Thanks,
Stefan
From: Jonathan Nieder <hidden> Date: 2017-05-04 19:19:35
Hi,
Junio C Hamano wrote:
Duy Nguyen [off-list ref] writes:
quoted
I attempted the same thing once or twice in the past, had the same
impression and dropped it. But I think it's good to get rid of cache_*
macros, at least outside builtin/ directory.
One thing that needs to be kept in mind is that a mechanical
replacement of active_cache[] to the_index.cache[] in the
infrastructure code does not get us closer to such an improvement.
In fact, such a patch only has negative value (keep reading until
the end of the paragraph that begins with "One thing is certain" to
understand why).
The entry point to a codechain for an important infrastructure code
that can currently only work with the_index needs to be identified.
There may be many. They need to be made to take an index_state
instance and to pass it through the callchain. That kind of change
would be an improvement, and will get us closer to mark the
resulting code with NO_THE_INDEX_COMP thing.
Thanks for articulating this. I agree with this general goal.
[...]
But one thing is certain. Many users of the API, just like a lot of
builtin/ code works only with the_index today, will not have to work
with a non-default repository or a non-default index_state. Only
the more advanced and complex "multi-repo" Porcelains would have to
care. Keeping active_cache[], active_nr, etc. abstraction would
isolate the simpler callers from the inevitable code churn we would
need while getting the "repository" abstraction right. This is why
I see that you understood the issues well when you said "builtiln/".
One small change we could make is to reverse the sense of the
NO_THE_INDEX_COMPATIBILITY_MACROS macro. That way, new library code
authors have to define USE_THE_INDEX_COMPATIBILITY_MACROS at the top
of the file, providing a hint to reviewers that they are using
the_index implicitly instead of relying on explicit repository or
index parameters.
More generally, if we are willing to follow through then I see
Stefan's change as a step in the right direction. Sure, it replaces
calls like read_cache with read_the_index(&the_index, ...) which does
not change semantics and may not look like progress. But:
- uses of 'the_index' are easy to grep for and it is easy to
understand that they are using global state. In builtins, this is
not important (which may be an argument for making builtins get
USE_THE_INDEX_COMPATIBILITY_MACROS implicitly instead and
restricting this change to library code), but in libraries it
communicates what is happening to developers looking at the code.
It is like a /* NEEDSWORK */ comment, except in code instead of
comments.
See Go's context.TODO <https://golang.org/pkg/context/#TODO> for a
similar example in another set of projects.
- it makes code consistently use the term "index" instead of mixing
"index" and "cache". This makes code easier to understand for new
contributors.
- a minor thing: it means more of git is using functions instead of
macros. IDEs, grep habits, etc cope better with functions. That is
minor, though: the compatibility macros could easily be replaced
with compatibility inline functions to achieve the same effect.
[...]
Also, dropping compatibility macros at the end of the series is
unfriendly to fellow developers with WIPs that depend on the
traditional way of doing things.
Agreed with this as well --- for widely used APIs like these, it is
more friendly to decouple adapting callers (in a separate patch) from
the patch that removes the API.
That is, one way to do what this series attempts would be the
following:
1. rename variables that shadow the_index.
2. add coccinelle patches (or one coccinelle patch) to
contrib/coccinelle implementing *_cache -> *_index migration.
Is there a way to do this without making it fail "make coccicheck"?
3. migrate library code (but not builtins) using that semantic patch
4. make compatibility macros opt-in instead of opt-out
(USE_THE_INDEX_COMPATIBILITY_MACROS). Opt-in all builtins.
5. optional: change the compatibility macros to compatibility inline
functions, perhaps.
6. optional: rename *_cache to *_the_index for clarity, perhaps.
7. optional: migrate builtins, if there is a way to make the patches
for that look sensible.
Thoughts?
Jonathan
From: Johannes Schindelin <hidden> Date: 2017-05-05 14:31:59
Hi Stefan & Junio,
On Thu, 4 May 2017, Stefan Beller wrote:
So instead of a mechanical replacement, we'd rather want to
see "the_index" not appearing at all outside of builtins, which
implies two things:
* If done properly we can move the macros from cache.h to
e.g. builtin.h. That way future developers are less tempted
to use the cache_* macros in the library code.
Yessss!
* we'd have to pass through the_index from the builtin function
down to the library code, potentially going through multiple
function. For this it is unclear if we want to start this now, or wait
until Brandon presents his initial repository object struct, which
may be suited better for passing-around.
Or the other way round. I guess passing a struct index_state can be a
first step, and we can later convert it to struct repository. I fathom
that more places will need a struct repository parameter than a struct
index_state parameter. That is, if you first identify all the places where
the index_state parameter is required, it should make the struct
repository change easier.
So if I want to further look into refactoring, I'll have a look into
the object store and hold off sending a series that drops the_index.
quoted
Also, dropping compatibility macros at the end of the series is
unfriendly to fellow developers with WIPs that depend on the
traditional way of doing things. It is doubly insulting to them to
send such a series during the feature freeze period, when it is more
likely that they are holding off their WIP in an attempt to allow us
to concentrate more on what we should be, i.e. finding and fixing
possible regressions at the tip of 'master' to polish the upcoming
release and help the end users.
Personally I have not noticed large variations of patch volume
correlated to pre-release times.
Speaking for myself, I also use this "slow" time to prepare patch series
that should be submitted directly after a major version bump, patch series
like the timestamp_t one (with which I failed miserably: it got to a
usable state only now, very short *before* a major version bump).
But yes, part of why I set aside a substantial chunk of time to work on
the Coverity report was to prepare for the major version, to make sure
that we did not have anything blatant (like the difftool use-after-free,
which was embarrassing) in v2.13.0.
It may look as if the coverity patches do not focus on critical fixes, but
that's only because I did not find anything major in the Coverity report
(I looked primarily for Git for Windows-specific stuff, though, to be
honest).
Maybe it is a similar situation for other patch contributors: they tried
to focus on critical fixes and ended up not finding anything really
critical?
Having said that, I did see a little shift toward cppcheck & sparse
related patches ;-)
Ciao,
Dscho
From: Brandon Williams <hidden> Date: 2017-05-05 17:20:57
On 05/05, Johannes Schindelin wrote:
Hi Stefan & Junio,
On Thu, 4 May 2017, Stefan Beller wrote:
quoted
So instead of a mechanical replacement, we'd rather want to
see "the_index" not appearing at all outside of builtins, which
implies two things:
* If done properly we can move the macros from cache.h to
e.g. builtin.h. That way future developers are less tempted
to use the cache_* macros in the library code.
Yessss!
quoted
* we'd have to pass through the_index from the builtin function
down to the library code, potentially going through multiple
function. For this it is unclear if we want to start this now, or wait
until Brandon presents his initial repository object struct, which
may be suited better for passing-around.
Or the other way round. I guess passing a struct index_state can be a
first step, and we can later convert it to struct repository. I fathom
that more places will need a struct repository parameter than a struct
index_state parameter. That is, if you first identify all the places where
the index_state parameter is required, it should make the struct
repository change easier.
Exactly this. I have a local series which converts ls-files to use a
repository struct but it turns out, for that to work, dir.c needs to be
converted to take in an index_state struct for fill_directory(). So I
then started working on doing that conversion and hopefully will have
something clean enough to send out later today for people to comment on.
--
Brandon Williams