On 6/28/2017 1:11 AM, Christian Couder wrote:
On Sat, Jun 10, 2017 at 3:40 PM, Ben Peart <peartben@xxxxxxxxx> wrote:
Changes from V4 include:
...
I took a look at this patch series except the last patch ([PATCH v5
7/7] fsmonitor: add a performance test) as Junio reviewed it already,
and had only a few comments on patches 3/7 and 4/7.
I am still not convinced by the discussions following v2
(http://public-inbox.org/git/20170518201333.13088-1-benpeart@xxxxxxxxxxxxx/)
about using a hook instead of for example a "core.fsmonitorcommand".
I think using a hook is not necessary and might not be a good match
for later optimizations. For example people might want to use a
library or some OS specific system calls to do what the hook does.
AEvar previously reported some not so great performance numbers on
some big Booking.com boxes with a big monorepo and it seems that using
taskset for example to make sure that the hook is run on the same CPU
improves these numbers significantly. So avoiding to run a separate
process can be important in some cases.
Using a hook is the only pattern I've seen in git that provides a way to
enable OS specific calls. I used a hook so that different file
monitoring services could be plugged in depending on the OS or tools
available. The Watchman integration script was mostly intended as a
sample that could be used where Watchman is available and works well.
I had not heard about the taskset issues you mention above. If there is
something else that can be done to make this work better, please let me
know the details.
If I'm understanding you correctly, you are suggesting that someone
should be able to configure a setting (core.fsmonitorcommand) that gives
a custom command line that would be run instead of running the
query-fsmonitor hook.
I'm not entirely sure how that should work. There are command line
options that need to be passed (currently the interface version as well
as the current clock in nanoseconds). How would those passed when using
the custom command?
Is it OK to just append them to the given command line? Does there need
to be some substitution token to indicate where they should be inserted
(ie "mycustomcommand --custom --options %version% --more-options
%timestamp%"). Are there any other tokens that should be supported (ie
PID or processor mask?).
Is there a design pattern already used somewhere in git that I can
follow or is this all blazing a new trail?
Thanks for continuing to look into this. Feedback is good!
