{"thread":{"id":"54479","subject":"[PATCH 0/2] fsmonitor inline / testing cleanup","startedAt":"2020-10-21T18:04:39Z","lastAt":"2020-10-22T20:59:32Z","messageCount":18,"participants":["Nipunn Koorapati via GitGitGadget","Alex Vandiver via GitGitGadget","Taylor Blau","Junio C Hamano","Nipunn Koorapati"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"408075","messageId":"pull.767.git.1603303474.gitgitgadget@gmail.com","threadId":"54479","inReplyTo":null,"subject":"[PATCH 0/2] fsmonitor inline / testing cleanup","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-21T18:04:32Z","receivedAt":"2020-10-21T18:04:39Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Credit to alexmv again - I'm rebasing these changes from a couple years ago\nfor contribution.\n\nFull comments are available here -\nhttps://public-inbox.org/git/01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com/\nTo summarize the relevant points\n\nRe: Inlining mark_fsmonitor_[in]valid peartben said\n\nI'm fine with these not being inline. I was attempting to minimize the \nperformance impact of the fsmonitor code when it was not even turned on. \n Inlineing these functions allowed it to be kept to a simple test but I \nsuspect (especially with modern optimizing compilers) that the overhead \nof calling a function to do that test is negligible.\n\nRe test-dump-fsmonitor peartben suggested keeping the +- syntax as well as\nthe summary counts dscho suggested dumping the invalid entries\n\nAlex Vandiver (2):\n fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid\n fsmonitor: make output of test-dump-fsmonitor more concise\n\n fsmonitor.c | 19 +++++++++++++++++++\n fsmonitor.h | 18 ++----------------\n t/helper/test-dump-fsmonitor.c | 14 ++++++++++++--\n 3 files changed, 33 insertions(+), 18 deletions(-)\n\n\nbase-commit: 69986e19ffcfb9af674ae5180689ab7bbf92ed28\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-767%2Fnipunn1313%2Ffsmonitor-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-767/nipunn1313/fsmonitor-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/767\n-- \ngitgitgadget\n"},{"id":"408076","messageId":"598521091a54caa73556b8bc9caf552d3216ba63.1603303474.git.gitgitgadget@gmail.com","threadId":"54479","inReplyTo":"pull.767.git.1603303474.gitgitgadget@gmail.com","subject":"[PATCH 2/2] fsmonitor: make output of test-dump-fsmonitor more concise","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-21T18:04:34Z","receivedAt":"2020-10-21T18:04:41Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nAfter displaying one very long line, summarize the contents of that\nline. The tests do not currently rely on any content except the first\nline (\"no fsmonitor\" / \"fsmonitor last update\").\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n t/helper/test-dump-fsmonitor.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex 975f0ac890..a42e402bf8 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -4,7 +4,7 @@\n int cmd__dump_fsmonitor(int ac, const char **av)\n {\n \tstruct index_state *istate = the_repository->index;\n-\tint i;\n+\tint i, valid = 0;\n \n \tsetup_git_directory();\n \tif (do_read_index(istate, the_repository->index_file, 0) < 0)\n@@ -15,8 +15,18 @@ int cmd__dump_fsmonitor(int ac, const char **av)\n \t}\n \tprintf(\"fsmonitor last update %s\\n\", istate->fsmonitor_last_update);\n \n-\tfor (i = 0; i < istate->cache_nr; i++)\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n+\t\tif (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)\n+\t\t\tvalid++;\n+\t}\n+\n+\tprintf(\"\\n valid: %d\\n\", valid);\n+\tprintf(\" invalid: %d\\n\", istate->cache_nr - valid);\n+\n+\tfor (i = 0; i < istate->cache_nr; i++)\n+\t\tif (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n+\t\t\tprintf(\" - %s\\n\", istate->cache[i]->name);\n \n \treturn 0;\n }\n-- \ngitgitgadget\n"},{"id":"408077","messageId":"049989652cefb90304e711dbfe354b55a5a71f41.1603303474.git.gitgitgadget@gmail.com","threadId":"54479","inReplyTo":"pull.767.git.1603303474.gitgitgadget@gmail.com","subject":"[PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-21T18:04:33Z","receivedAt":"2020-10-21T18:04:43Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nThese were inline'd when they were first introduced, presumably as an\noptimization for cases when they were called in tight loops. This\ncomplicates using these functions, as untracked_cache_invalidate_path\nis defined in dir.h.\n\nLeave the inline'ing up to the compiler's decision, for ease of use.\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n fsmonitor.c | 19 +++++++++++++++++++\n fsmonitor.h | 18 ++----------------\n 2 files changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex ca031c3abb..e120b3c5a9 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -287,6 +287,25 @@ void refresh_fsmonitor(struct index_state *istate)\n \tistate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);\n }\n \n+void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)\n+{\n+\tif (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n+\t\tistate->cache_changed = 1;\n+\t\tce->ce_flags |= CE_FSMONITOR_VALID;\n+\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n+\t}\n+}\n+\n+void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)\n+{\n+\tif (core_fsmonitor) {\n+\t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n+\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n+\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n+\t}\n+}\n+\n+\n void add_fsmonitor(struct index_state *istate)\n {\n \tunsigned int i;\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex 739318ab6d..6249020692 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -49,14 +49,7 @@ void refresh_fsmonitor(struct index_state *istate);\n * called any time the cache entry has been updated to reflect the\n * current state of the file on disk.\n */\n-static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)\n-{\n-\tif (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n-\t\tistate->cache_changed = 1;\n-\t\tce->ce_flags |= CE_FSMONITOR_VALID;\n-\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n-\t}\n-}\n+extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce);\n \n /*\n * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate\n@@ -65,13 +58,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache\n * trigger an lstat() or invalidate the untracked cache for the\n * corresponding directory\n */\n-static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)\n-{\n-\tif (core_fsmonitor) {\n-\t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n-\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n-\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n-\t}\n-}\n+extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);\n \n #endif\n-- \ngitgitgadget\n\n"},{"id":"408099","messageId":"20201021205208.GA1270359@nand.local","threadId":"54479","inReplyTo":"pull.767.git.1603303474.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] fsmonitor inline / testing cleanup","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-21T20:52:08Z","receivedAt":"2020-10-21T20:52:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Nipunn,\n\nOn Wed, Oct 21, 2020 at 06:04:32PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> Credit to alexmv again - I'm rebasing these changes from a couple years ago\n> for contribution.\n>\n> Full comments are available here -\n> https://public-inbox.org/git/01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com/\n> To summarize the relevant points\n\nI'm fine with both of these patches, but it may help to have a bit\nmore information about how they will be used. Presumably more patches\nare coming that make use of the new public functions, but it'd be good\nto know a little bit of why these changes are necessary.\n\nThanks,\nTaylor\n"},{"id":"408101","messageId":"20201021205537.GB1270359@nand.local","threadId":"54479","inReplyTo":"049989652cefb90304e711dbfe354b55a5a71f41.1603303474.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-21T20:55:37Z","receivedAt":"2020-10-21T20:55:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 21, 2020 at 06:04:33PM +0000, Alex Vandiver via GitGitGadget wrote:\n> From: Alex Vandiver \n>\n> These were inline'd when they were first introduced, presumably as an\n> optimization for cases when they were called in tight loops. This\n> complicates using these functions, as untracked_cache_invalidate_path\n> is defined in dir.h.\n>\n> Leave the inline'ing up to the compiler's decision, for ease of use.\n\nLetting the compiler inline these is fine, but...\n\n> diff --git a/fsmonitor.h b/fsmonitor.h\n> index 739318ab6d..6249020692 100644\n> --- a/fsmonitor.h\n> +++ b/fsmonitor.h\n> @@ -49,14 +49,7 @@ void refresh_fsmonitor(struct index_state *istate);\n> * called any time the cache entry has been updated to reflect the\n> * current state of the file on disk.\n> */\n> -static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)\n> -{\n> -\tif (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n> -\t\tistate->cache_changed = 1;\n> -\t\tce->ce_flags |= CE_FSMONITOR_VALID;\n> -\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n> -\t}\n> -}\n> +extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce);\n>\n> /*\n> * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate\n> @@ -65,13 +58,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache\n> * trigger an lstat() or invalidate the untracked cache for the\n> * corresponding directory\n> */\n> -static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)\n> -{\n> -\tif (core_fsmonitor) {\n> -\t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n> -\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n> -\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n> -\t}\n> -}\n> +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);\n>\n> #endif\n\nAny reason that these need to be externed explicitly? Note that these\nfunctions are already externed by default since you haven't said\notherwise (and for no other reason than this'd be the only explicitly\nexterned function in fsmonitor.h).\n\nThanks,\nTaylor\n"},{"id":"408103","messageId":"xmqqo8kv5l2x.fsf@gitster.c.googlers.com","threadId":"54479","inReplyTo":"20201021205537.GB1270359@nand.local","subject":"Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-21T21:24:22Z","receivedAt":"2020-10-21T21:24:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);\n> ...\n> Any reason that these need to be externed explicitly? Note that these\n> functions are already externed by default since you haven't said\n> otherwise (and for no other reason than this'd be the only explicitly\n> externed function in fsmonitor.h).\n\nPossibly due to the recent discussion?\n\nhttps://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/\n\n"},{"id":"408105","messageId":"20201021213136.GA1877888@nand.local","threadId":"54479","inReplyTo":"xmqqo8kv5l2x.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-21T21:31:36Z","receivedAt":"2020-10-21T21:31:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 21, 2020 at 02:24:22PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n> > Any reason that these need to be externed explicitly? Note that these\n> > functions are already externed by default since you haven't said\n> > otherwise (and for no other reason than this'd be the only explicitly\n> > externed function in fsmonitor.h).\n>\n> Possibly due to the recent discussion?\n>\n> https://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/\n\nAh, thanks. I remember the thread, but I wasn't sure where the\ndiscussion ended up. After re-reading it, it sounds like new function\ndeclarations in header files should be prefixed with 'extern' (making\nthis patch correct as it already is).\n\nTangential to this discussion: are you still expecting a tree-wide\nchange to start use extern everywhere?\n\nThanks,\nTaylor\n"},{"id":"408106","messageId":"xmqqk0vj5kg0.fsf@gitster.c.googlers.com","threadId":"54479","inReplyTo":"20201021213136.GA1877888@nand.local","subject":"Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-21T21:38:07Z","receivedAt":"2020-10-21T21:38:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Tangential to this discussion: are you still expecting a tree-wide\n> change to start use extern everywhere?\n\nI think before we start opening the tree for new topics is the best\ntime to do so, if we were to follow through, but after we have dealt\nwith brown-paper-bag fixes to the release. The Makefile tweak for\nthe skip-dashed thing is the only one for 2.29, I think, so ...\n\nThanks.\n\n\n"},{"id":"408125","messageId":"CAN8Z4-WxLcBzxrRsj4sRmMAKRZyLXpMA82r0Ce7RUiegtHWQ3Q@mail.gmail.com","threadId":"54479","inReplyTo":"20201021205208.GA1270359@nand.local","subject":"Re: [PATCH 0/2] fsmonitor inline / testing cleanup","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-21T23:15:02Z","receivedAt":"2020-10-21T23:17:06Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> I'm fine with both of these patches, but it may help to have a bit\n> more information about how they will be used. Presumably more patches\n> are coming that make use of the new public functions, but it'd be good\n> to know a little bit of why these changes are necessary.\n\nI believe the externs are just there to avoid pulling in `dir.h` from\nthe include file - since\nit's only needed in the implementation. I believe at the time\n(12/2017) `dir.h` was not imported from\n`fsmonitor.h`, but it does appear imported now. I've eliminated the\nimport of `dir.h` as it\nno longer appears necessary - which I will include in the next\niteration of this diff.\n\nThe test helper merely makes it easier to debug fsmonitor tests - will\nbe useful to any\ndeveloper working on fsmonitor related changes. I have an upcoming one\nrelated to\nfsmonitor in git checkout, which I've also revived, but I'm not 100%\nsure I'll get it through,\nbut regardless, I believe this test-debugging change will be good.\n\n--Nipunn\n"},{"id":"408126","messageId":"CAN8Z4-WY8Q-VCujRJFmEpDwX8OYfRYFAgam0CFbQVwDTRU+DKw@mail.gmail.com","threadId":"54479","inReplyTo":"20201021205537.GB1270359@nand.local","subject":"Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-21T23:22:54Z","receivedAt":"2020-10-21T23:23:07Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> Letting the compiler inline these is fine, but...\n>\n> Any reason that these need to be externed explicitly? Note that these\n> functions are already externed by default since you haven't said\n> otherwise (and for no other reason than this'd be the only explicitly\n> externed function in fsmonitor.h).\n\nDid not have a reason or strong opinion here. It was this way, because this was\nthe way alexmv used it originally - but it does compile in either manner. The\nthread Junio linked does seem to indicate preference for extern to avoid\nconfusion.\n\n--Nipunn\n"},{"id":"408130","messageId":"8ff657ded147f23e9db96c21205771e09dac9dca.1603326066.git.gitgitgadget@gmail.com","threadId":"54479","inReplyTo":"pull.767.v2.git.1603326066.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] fsmonitor: make output of test-dump-fsmonitor more concise","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-22T00:21:06Z","receivedAt":"2020-10-22T00:21:13Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nAfter displaying one very long line, summarize the contents of that\nline. The tests do not currently rely on any content except the first\nline (\"no fsmonitor\" / \"fsmonitor last update\").\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n t/helper/test-dump-fsmonitor.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex 975f0ac890..a42e402bf8 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -4,7 +4,7 @@\n int cmd__dump_fsmonitor(int ac, const char **av)\n {\n \tstruct index_state *istate = the_repository->index;\n-\tint i;\n+\tint i, valid = 0;\n \n \tsetup_git_directory();\n \tif (do_read_index(istate, the_repository->index_file, 0) < 0)\n@@ -15,8 +15,18 @@ int cmd__dump_fsmonitor(int ac, const char **av)\n \t}\n \tprintf(\"fsmonitor last update %s\\n\", istate->fsmonitor_last_update);\n \n-\tfor (i = 0; i < istate->cache_nr; i++)\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n+\t\tif (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)\n+\t\t\tvalid++;\n+\t}\n+\n+\tprintf(\"\\n valid: %d\\n\", valid);\n+\tprintf(\" invalid: %d\\n\", istate->cache_nr - valid);\n+\n+\tfor (i = 0; i < istate->cache_nr; i++)\n+\t\tif (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n+\t\t\tprintf(\" - %s\\n\", istate->cache[i]->name);\n \n \treturn 0;\n }\n-- \ngitgitgadget\n"},{"id":"408131","messageId":"pull.767.v2.git.1603326066.gitgitgadget@gmail.com","threadId":"54479","inReplyTo":"pull.767.git.1603303474.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] fsmonitor inline / testing cleanup","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-22T00:21:04Z","receivedAt":"2020-10-22T00:21:14Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"UPDATE SINCE v1\n\n * Removed include of dir.h from fsmonitor.h as it's no longer needed\n\nCredit to alexmv again - I'm rebasing these changes from a couple years ago\nfor contribution.\n\nFull comments are available here -\nhttps://public-inbox.org/git/01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com/\nTo summarize the relevant points\n\nRe: Inlining mark_fsmonitor_[in]valid peartben said\n\nI'm fine with these not being inline. I was attempting to minimize the \nperformance impact of the fsmonitor code when it was not even turned on. \n Inlineing these functions allowed it to be kept to a simple test but I \nsuspect (especially with modern optimizing compilers) that the overhead \nof calling a function to do that test is negligible.\n\nRe test-dump-fsmonitor peartben suggested keeping the +- syntax as well as\nthe summary counts dscho suggested dumping the invalid entries\n\nAlex Vandiver (2):\n fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid\n fsmonitor: make output of test-dump-fsmonitor more concise\n\n fsmonitor.c | 19 +++++++++++++++++++\n fsmonitor.h | 19 ++-----------------\n t/helper/test-dump-fsmonitor.c | 14 ++++++++++++--\n 3 files changed, 33 insertions(+), 19 deletions(-)\n\n\nbase-commit: 69986e19ffcfb9af674ae5180689ab7bbf92ed28\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-767%2Fnipunn1313%2Ffsmonitor-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-767/nipunn1313/fsmonitor-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/767\n\nRange-diff vs v1:\n\n 1: 049989652c ! 1: ab9c330ca8 fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid\n @@ fsmonitor.c: void refresh_fsmonitor(struct index_state *istate)\n \tunsigned int i;\n \n ## fsmonitor.h ##\n +@@\n + #define FSMONITOR_H\n + \n + #include \"cache.h\"\n +-#include \"dir.h\"\n + \n + extern struct trace_key trace_fsmonitor;\n + \n @@ fsmonitor.h: void refresh_fsmonitor(struct index_state *istate);\n * called any time the cache entry has been updated to reflect the\n * current state of the file on disk.\n 2: 598521091a = 2: 8ff657ded1 fsmonitor: make output of test-dump-fsmonitor more concise\n\n-- \ngitgitgadget\n"},{"id":"408132","messageId":"ab9c330ca804eab6061d997bb5f216e48b199876.1603326066.git.gitgitgadget@gmail.com","threadId":"54479","inReplyTo":"pull.767.v2.git.1603326066.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-22T00:21:05Z","receivedAt":"2020-10-22T00:21:15Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nThese were inline'd when they were first introduced, presumably as an\noptimization for cases when they were called in tight loops. This\ncomplicates using these functions, as untracked_cache_invalidate_path\nis defined in dir.h.\n\nLeave the inline'ing up to the compiler's decision, for ease of use.\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n fsmonitor.c | 19 +++++++++++++++++++\n fsmonitor.h | 19 ++-----------------\n 2 files changed, 21 insertions(+), 17 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex ca031c3abb..e120b3c5a9 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -287,6 +287,25 @@ void refresh_fsmonitor(struct index_state *istate)\n \tistate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);\n }\n \n+void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)\n+{\n+\tif (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n+\t\tistate->cache_changed = 1;\n+\t\tce->ce_flags |= CE_FSMONITOR_VALID;\n+\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n+\t}\n+}\n+\n+void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)\n+{\n+\tif (core_fsmonitor) {\n+\t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n+\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n+\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n+\t}\n+}\n+\n+\n void add_fsmonitor(struct index_state *istate)\n {\n \tunsigned int i;\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex 739318ab6d..313a35fdc8 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -2,7 +2,6 @@\n #define FSMONITOR_H\n \n #include \"cache.h\"\n-#include \"dir.h\"\n \n extern struct trace_key trace_fsmonitor;\n \n@@ -49,14 +48,7 @@ void refresh_fsmonitor(struct index_state *istate);\n * called any time the cache entry has been updated to reflect the\n * current state of the file on disk.\n */\n-static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)\n-{\n-\tif (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n-\t\tistate->cache_changed = 1;\n-\t\tce->ce_flags |= CE_FSMONITOR_VALID;\n-\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n-\t}\n-}\n+extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce);\n \n /*\n * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate\n@@ -65,13 +57,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache\n * trigger an lstat() or invalidate the untracked cache for the\n * corresponding directory\n */\n-static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)\n-{\n-\tif (core_fsmonitor) {\n-\t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n-\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n-\t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n-\t}\n-}\n+extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);\n \n #endif\n-- \ngitgitgadget\n\n"},{"id":"408178","messageId":"20201022174043.GA775513@nand.local","threadId":"54479","inReplyTo":"pull.767.v2.git.1603326066.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-22T17:40:43Z","receivedAt":"2020-10-22T17:40:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Nipunn,\n\nOn Thu, Oct 22, 2020 at 12:21:04AM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> UPDATE SINCE v1\n>\n> * Removed include of dir.h from fsmonitor.h as it's no longer needed\n\nThis version all looks sensible to me.\n\nI'm still iffy on whether or not this series makes sense to apply\nwithout the rest of the code that depends on it, but I'll leave that up\nto Junio whether he wants to take the series as it is now, or wait for\nother patches to come in on top.\n\nIn either case, these two patches are:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"408181","messageId":"xmqqblgum7qk.fsf@gitster.c.googlers.com","threadId":"54479","inReplyTo":"20201022174043.GA775513@nand.local","subject":"Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-22T18:32:51Z","receivedAt":"2020-10-22T18:32:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Hi Nipunn,\n>\n> On Thu, Oct 22, 2020 at 12:21:04AM +0000, Nipunn Koorapati via GitGitGadget wrote:\n>> UPDATE SINCE v1\n>>\n>> * Removed include of dir.h from fsmonitor.h as it's no longer needed\n>\n> This version all looks sensible to me.\n>\n> I'm still iffy on whether or not this series makes sense to apply\n> without the rest of the code that depends on it, but I'll leave that up\n> to Junio whether he wants to take the series as it is now, or wait for\n> other patches to come in on top.\n\nSorry but I am not sure what you mean by \"the code that depends on\nit\". Are these two functions unused anywhere in the code? If so,\nthe right way to clean them up may not be to turn them from inline\nto a proper definition, but to remove them ;-). \n\nIf they have existing callers and it can be demonstrated that their\ncallers do not benefit from them being inline, that by itself is a\nworthy clean-up, without adding any more callers, no?\n\nConfused...\n\n> In either case, these two patches are:\n>\n> Reviewed-by: Taylor Blau \n>\n> Thanks,\n> Taylor\n"},{"id":"408182","messageId":"20201022183822.GA781760@nand.local","threadId":"54479","inReplyTo":"xmqqblgum7qk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-22T18:38:22Z","receivedAt":"2020-10-22T18:38:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 22, 2020 at 11:32:51AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n> > I'm still iffy on whether or not this series makes sense to apply\n> > without the rest of the code that depends on it, but I'll leave that up\n> > to Junio whether he wants to take the series as it is now, or wait for\n> > other patches to come in on top.\n>\n> Sorry but I am not sure what you mean by \"the code that depends on\n> it\". Are these two functions unused anywhere in the code? If so,\n> the right way to clean them up may not be to turn them from inline\n> to a proper definition, but to remove them ;-).\n>\n> If they have existing callers and it can be demonstrated that their\n> callers do not benefit from them being inline, that by itself is a\n> worthy clean-up, without adding any more callers, no?\n>\n> Confused...\n\nSorry for the confusion. I mean the following:\n\n - These functions have existing callers that Nipunn claims do not need\n to be explicitly inlined.\n\n - These functions are being moved to be part of the fsmonitor public\n interface (presumably so that new callers can be added).\n\n...And I was wondering whether you wanted to wait for new callers\nbefore applying these to your tree.\n\nThanks,\nTaylor\n"},{"id":"408184","messageId":"xmqq7drim5st.fsf@gitster.c.googlers.com","threadId":"54479","inReplyTo":"20201022183822.GA781760@nand.local","subject":"Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-22T19:14:42Z","receivedAt":"2020-10-22T19:14:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Sorry for the confusion. I mean the following:\n>\n> - These functions have existing callers that Nipunn claims do not need\n> to be explicitly inlined.\n\nI guess \"claims\" is the key phrase in your responsehere. Do you\nfeel that the claim is not sufficiently substantiated?\n\nThose without fsmonitor would pay the call/return cost for no good\nreason if core_fsmonitor is not set, and checking that on the caller\nside may make a big difference. How big? That needs measurement.\n\nThis is a tangent, but with or without inlining, I find it iffy to\nsee that untracked_cache_invalidate_path() is called only when\nfsmonitor is in use. Does untracked_cache depend on fsmonitor for\nits correct operation? Why is it OK not to invlidate when the\ncaller would tell fsmonitor that a path is invalid if fsmonitor were\nin use? The call is a statement of fact that the path is no longer\nvalid, and that bit of information would be useful to the parts of\nthe system outside fsmonitor, no? Puzzled....\n\n> - These functions are being moved to be part of the fsmonitor public\n> interface (presumably so that new callers can be added).\n\nThey used to be implemented as static inline functions in the\nfsmonitor.h header file, so they have been part of the public\ninterface anyway. Anybody that includes fsmonitor.h can use it,\nwith or without the patch. So I think this one would not be\na problem.\n\n> ...And I was wondering whether you wanted to wait for new callers\n> before applying these to your tree.\n\nThanks.\n\nI still do not know about the \"should the inline be kept\" question.\nThe proposed log message for the commit does not explain (let alone\njustify) why \"optimization\" is unneeded for the fuctions in the\nfirst place, which does not help.\n\n\n\n"},{"id":"408190","messageId":"CAN8Z4-Vb3qc7eyzczEC7hcf3DmHEXkcV1AGRfC_L0uFKDU2W5A@mail.gmail.com","threadId":"54479","inReplyTo":"xmqq7drim5st.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-22T20:59:15Z","receivedAt":"2020-10-22T20:59:32Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> from Taylor\n> I'm still iffy on whether or not this series makes sense to apply\n> without the rest of the code that depends on it\n\nSorry for confusion. I don't think we should assume there is more code coming\nrelated to this. I think this is intended to stand on its own.\nIt's not a required dependency either. Rather, it's motivated by\nsimplicity\n- remove the dir.h dependency from fsmonitor.h.\n- Keep implementation in fsmonitor.c and definitions in fsmonitor.h\n\n> From Junio\n> Those without fsmonitor would pay the call/return cost for no good\n> reason if core_fsmonitor is not set, and checking that on the caller\n> side may make a big difference. How big? That needs measurement.\n\nNoted! This is not called out or measured - it is simply assumed based\non earlier\nconversation. I should be able to run the fsmonitor perf suite before/after this\nchange and include the results in the commit message.\n\n> This is a tangent, but with or without inlining, I find it iffy to\n> see that untracked_cache_invalidate_path() is called only when\n> fsmonitor is in use. Does untracked_cache depend on fsmonitor for\n> its correct operation? Why is it OK not to invlidate when the\n> caller would tell fsmonitor that a path is invalid if fsmonitor were\n> in use? The call is a statement of fact that the path is no longer\n> valid, and that bit of information would be useful to the parts of\n> the system outside fsmonitor, no? Puzzled....\n\nI did some source diving in an attempt to understand what's happening here.\nI believe that untracked_cache_invalidate_path() is called in dir.c\nwhenever an entry is added or removed from\na directory.\nThis is an additional call when fsmonitor is enabled - because\nfsmonitor's whole purpose\nis to avoid the lstat on the other path. There is a nice explanation\nin the original commit message\n\nCommit 883e248b (fsmonitor: teach git to optionally utilize a file\nsystem monitor to speed up detecting new or changed files.,\n2017-09-22)\n\n--Nipunn\n"}]}