On Tue, Aug 6, 2019 at 4:16 AM Christian Couder <christian.couder@xxxxxxxxx> wrote: > > Hi Matheus, > > On Tue, Aug 6, 2019 at 4:54 AM Matheus Tavares Bernardino > <matheus.bernardino@xxxxxx> wrote: > > > > Here's my report from last week: > > https://matheustavares.gitlab.io/posts/week-11-wip-grep-protecting-textconv-and-submodules > > Thank you for another great report! > > > I'm working to protect the operations I left behind on the first > > version of the patchset[1]. And for that, I used a lot of the code Duy > > provided[2] me as an example in the early days of this project. The > > race conditions are now majorly gone, but the patches still need some > > refactoring and there are still some problems to overcome. > > This looks good to me! I think you are now way past the point where > you know much more about this than me, but anyway the following might > give you some ideas or perhaps in some other ways help you: > > - About "Try to remove the obj_read_lock", which is the last point > on your TODO list, it doesn't look necessary before you submit a patch > series, unless there is no performance improvement without it. You > could, as far as I can tell, do it in a later patch series. > > - I guess " --recurse-submodules" is not used very often with git > grep. For example I never use it on the Git repo even when I perhaps > should. So if it would simplify things to just disable the > improvements you made when " --recurse-submodules" is used, then you > might consider doing that. You could later send another patch series > that would enable your improvements when " --recurse-submodules" is > used. Thanks for the suggestions, Christian. So, I think I could finish and send the patchset without the optimization for --recurse-submodules and --textconv, while I don't finish support for them, right? And I think 'obj_read_lock' may be removed in a future patch, as you said, because we already have good speedup without removing it. Finally, should I also add a "NOTES" section to Documentation/git-grep.txt saying which options --threads is not compatible with? Thanks, Matheus
