{"thread":{"id":"42597","subject":"Re attr API further revamp","startedAt":"2016-06-16T06:55:52Z","lastAt":"2016-06-16T06:55:52Z","messageCount":3,"participants":["Junio C Hamano","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"289165","messageId":"xmqqtwgwbygg.fsf@gitster.mtv.corp.google.com","threadId":"42597","inReplyTo":null,"subject":"Re attr API further revamp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-13T20:55:43Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I hate to be doing this, but we need yet another revamp to the attr\nAPI that affects all the callers.\n\nIn the original design, a codepath that wants to check attributes\nrepeatedly for many paths (e.g. \"convert\" that wants to see what\ncrlf, ident, filter, eol and text attributes are set to for paths\nthat it is asked to munge the blob contents for) used to allocate\n\n\tstatic struct git_attr_check {\n        \tstruct git_attr *attr;\n                const char *value;\n\t} ccheck[NUM_ATTRS];\n\nand populated the array when the first time the codepath was\nentered, e.g.\n\n\tif (!ccheck[0].attr) {\n        \tfor (i = 0; i < NUM_ATTRS; i++)\n                \tccheck[i].attr = git_attr(...);\n\t}\n\nand then make a call to the API to ask for these attributes in\nccheck[] for a path, i.e.\n\n\tgit_attr_check(path, NUM_ATTRS, ccheck);\n\nThe API function will fill in the ccheck[].value fields and the\ncaller will read from there how each attribute is set for the path.\n\nThis was updated with recent jc/attr topic so that attr_check\nstructure can be enriched to keep _more_ state, such as the\npre-parsed representation of all the contents of the relevant\n.gitattributes files, which currently is held in a program-side\nsingleton instance called attr_stack, partly in preparation for\nStefan's sb/pathspec-label topic that is expected to place a lot\nheavier load to the attribute subsystem.\n\nA caller of the API after that update would do more like\n\n  \tstatic struct git_attr_check *ccheck;\n\n\tif (!ccheck)\n        \tccheck = git_attr_check_initl(...);\n\tgit_attr_check(path, ccheck);\n\tfor (i = 0; i < NUM_ATTRS; i++)\n        \tif (ccheck->check[i].value == ATTR_UNSET)\n\t\t\t... do the Unset thing ...\n\nThis however is not thread-safe for obvious two reasons.\n\n * Two threads can simultanously enter this section of the code and \n   end up initializing ccheck twice;\n\n * Even though ccheck[].attr are constant for the purpose of this\n   codepath (i.e. all threads passing through are interested in\n   checking the same set of attributes), ccheck[].value fields\n   cannot be shared across simultaneous threads like the above\n   snippet does.\n\nSo it appears that the final API visible from the callers needs to\nbe updated further, perhaps something like:\n\n\n\tstatic struct git_attr_check *ccheck;\n\tconst char *values[NUM_ATTRS];\n\n\tgit_attr_check_initl(&ccheck, \"crlf\", \"ident\", ..., NULL);\n\tgit_attr_check(path, ccheck, values);\n\nwhere git_attr_check_initl() would do the \"if ccheck is NULL then\ninitialize it to an instance\" atomically in threading environment,\nand git_attr_check() returns its finding to values[] array the\ncalling thread exclusively owns, while sharing the input ccheck\nand the list of attributes the call inquires among threads.\n\nAlso unlike the earlier plan, attr_stack aka \"where in the directory\nhierarchy are we asking attributes for?\" will not be stored directly\nin git_attr_check structure.  It is likely that a thread-local\nstructure will be introduced hidden behind this API (i.e. the\ncallers do not have to be aware of it), and attr_stack will be\nregistered in this thread-local structure keyed by &ccheck, so that\n<thread, callsite> pair can have attr_stack instance of its own.\n\nThis is because a single attr_stack per ccheck will not work as an\noptimization mechanism when multi-threaded.  Two threads may be\nrunning the same convert() function, and they may be interested in\nthe same set of attributes (e.g. \"crlf\", \"ident\", etc.) but they\nwill be working on different parts of the tree, so having attr_stack\nper <thread, callsite> would make more sense.\n\nAll of which will be quite a lot of work, though, so do not expect\nthat it will appear by the end of this week ;-)\n\nThanks.\n"},{"id":"289167","messageId":"CAGZ79kYy4ic1nYgPEHLCfoUZYkzPw3bBq+8kTyRi13nNwQHGFw@mail.gmail.com","threadId":"42597","inReplyTo":"xmqqtwgwbygg.fsf@gitster.mtv.corp.google.com","subject":"Re: Re attr API further revamp","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-13T21:31:18Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 13, 2016 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> I hate to be doing this, but we need yet another revamp to the attr\n> API that affects all the callers.\n\nSo you don't mean origin/jc/attr-more by this?\n(Given that we have jc/attr and jc/attr-more, the third thing could go\nwith jc/even-more-attr. Though I do not propose that)\n\n>\n> In the original design, a codepath that wants to check attributes\n> repeatedly for many paths (e.g. \"convert\" that wants to see what\n> crlf, ident, filter, eol and text attributes are set to for paths\n> that it is asked to munge the blob contents for) used to allocate\n>\n>         static struct git_attr_check {\n>                 struct git_attr *attr;\n>                 const char *value;\n>         } ccheck[NUM_ATTRS];\n>\n> and populated the array when the first time the codepath was\n> entered, e.g.\n>\n>         if (!ccheck[0].attr) {\n>                 for (i = 0; i < NUM_ATTRS; i++)\n>                         ccheck[i].attr = git_attr(...);\n>         }\n>\n> and then make a call to the API to ask for these attributes in\n> ccheck[] for a path, i.e.\n>\n>         git_attr_check(path, NUM_ATTRS, ccheck);\n>\n> The API function will fill in the ccheck[].value fields and the\n> caller will read from there how each attribute is set for the path.\n>\n> This was updated with recent jc/attr topic so that attr_check\n> structure can be enriched to keep _more_ state, such as the\n> pre-parsed representation of all the contents of the relevant\n> .gitattributes files, which currently is held in a program-side\n> singleton instance called attr_stack, partly in preparation for\n> Stefan's sb/pathspec-label topic that is expected to place a lot\n> heavier load to the attribute subsystem.\n\nI do not quite expect that. Some numbers:\n(I am looking at https://android.googlesource.com/platform/manifest)\n* There are currently different 56 groups in that manifest\n* with an average of 1.5 groups for each entry that has at least one group,\n* or 0.88 groups per entry (which is one resulting file/submodule after the\n  conversion)\n\nFor reference:\n\n* git.git makes use of 1 attr (whitespace), in 2 value configurations\n* that is applied to each file.\n\nSo comparing one entry to the 1.5 or 0.88 above doesn't seem to\nbe that bad. That is what needs to be done on a per-file basis?\n\nWhat is different is the number of different attrs in use by about\n2 orders of magnitude, which needs to be collected upfront and stored\nin a smart way?\n\n>\n> A caller of the API after that update would do more like\n>\n>         static struct git_attr_check *ccheck;\n>\n>         if (!ccheck)\n>                 ccheck = git_attr_check_initl(...);\n>         git_attr_check(path, ccheck);\n>         for (i = 0; i < NUM_ATTRS; i++)\n>                 if (ccheck->check[i].value == ATTR_UNSET)\n>                         ... do the Unset thing ...\n\nThis is origin/jc/attr-more ?\n\n>\n> This however is not thread-safe for obvious two reasons.\n>\n>  * Two threads can simultanously enter this section of the code and\n>    end up initializing ccheck twice;\n>\n>  * Even though ccheck[].attr are constant for the purpose of this\n>    codepath (i.e. all threads passing through are interested in\n>    checking the same set of attributes), ccheck[].value fields\n>    cannot be shared across simultaneous threads like the above\n>    snippet does.\n>\n> So it appears that the final API visible from the callers needs to\n> be updated further, perhaps something like:\n>\n>\n>         static struct git_attr_check *ccheck;\n>         const char *values[NUM_ATTRS];\n>\n>         git_attr_check_initl(&ccheck, \"crlf\", \"ident\", ..., NULL);\n>         git_attr_check(path, ccheck, values);\n\nand later on each thread will do a\n\n        git_attr_thread_byebye(&ccheck) ?\n\nto free its thread local stuff or rather is the calling site responsible for\nfreeing up all of it after all threads are done?\n(This is probably too much of a detail question now?)\n\n>\n> where git_attr_check_initl() would do the \"if ccheck is NULL then\n> initialize it to an instance\" atomically in threading environment,\n> and git_attr_check() returns its finding to values[] array the\n> calling thread exclusively owns, while sharing the input ccheck\n> and the list of attributes the call inquires among threads.\n>\n> Also unlike the earlier plan, attr_stack aka \"where in the directory\n> hierarchy are we asking attributes for?\" will not be stored directly\n> in git_attr_check structure.  It is likely that a thread-local\n> structure will be introduced hidden behind this API (i.e. the\n> callers do not have to be aware of it), and attr_stack will be\n> registered in this thread-local structure keyed by &ccheck, so that\n> <thread, callsite> pair can have attr_stack instance of its own.\n>\n> This is because a single attr_stack per ccheck will not work as an\n> optimization mechanism when multi-threaded.  Two threads may be\n> running the same convert() function, and they may be interested in\n> the same set of attributes (e.g. \"crlf\", \"ident\", etc.) but they\n> will be working on different parts of the tree, so having attr_stack\n> per <thread, callsite> would make more sense.\n>\n> All of which will be quite a lot of work, though, so do not expect\n> that it will appear by the end of this week ;-)\n>\n> Thanks.\n>\n\nThanks for the heads up,\nStefan\n"},{"id":"289168","messageId":"xmqqk2hsbvpr.fsf@gitster.mtv.corp.google.com","threadId":"42597","inReplyTo":"CAGZ79kYy4ic1nYgPEHLCfoUZYkzPw3bBq+8kTyRi13nNwQHGFw@mail.gmail.com","subject":"Re: Re attr API further revamp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-13T21:54:56Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Mon, Jun 13, 2016 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> I hate to be doing this, but we need yet another revamp to the attr\n>> API that affects all the callers.\n>\n> So you don't mean origin/jc/attr-more by this?\n\nNot really; the tip of attr-more needs to be discarded after this\nrealization, but otherwise there is no API revamping in what is\nqueued on that topic.\n\n> (Given that we have jc/attr and jc/attr-more, the third thing could go\n> with jc/even-more-attr. Though I do not propose that)\n\nThe latter was merely because I couldn't rewind and rebuild the\nformer during the pre-release freeze.  The fixup! in the latter will\nbe squashed into the former, etc...\n\n>>         static struct git_attr_check *ccheck;\n>>         const char *values[NUM_ATTRS];\n>>\n>>         git_attr_check_initl(&ccheck, \"crlf\", \"ident\", ..., NULL);\n>>         git_attr_check(path, ccheck, values);\n>\n> and later on each thread will do a\n\nThis will have to do nothing; it is \"static\", and will be held to\nthe end of the lifetime of the program.\n\nYou _can_ allocate the check structure per thread and free them when\nthread exits, but the usage pattern that appear in convert.c is\noblivious to the threading.\n"}]}