Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2020, 19:14 UTC
Message-ID: <xmqq7drim5st.fsf@gitster.c.googlers.com>
In-Reply-To: <20201022183822.GA781760@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

> Sorry for the confusion. I mean the following:
>
>   - These functions have existing callers that Nipunn claims do not need
>     to be explicitly inlined.

I guess "claims" is the key phrase in your responsehere. Do you feel that the claim is not sufficiently substantiated?

Those without fsmonitor would pay the call/return cost for no good reason if core_fsmonitor is not set, and checking that on the caller side may make a big difference. How big? That needs measurement.

This is a tangent, but with or without inlining, I find it iffy to see that untracked_cache_invalidate_path() is called only when fsmonitor is in use. Does untracked_cache depend on fsmonitor for its correct operation? Why is it OK not to invlidate when the caller would tell fsmonitor that a path is invalid if fsmonitor were in use? The call is a statement of fact that the path is no longer valid, and that bit of information would be useful to the parts of the system outside fsmonitor, no? Puzzled....

>   - These functions are being moved to be part of the fsmonitor public
>     interface (presumably so that new callers can be added).

They used to be implemented as static inline functions in the fsmonitor.h header file, so they have been part of the public interface anyway. Anybody that includes fsmonitor.h can use it, with or without the patch. So I think this one would not be a problem.

> ...And I was wondering whether you wanted to wait for new callers
> before applying these to your tree.

Thanks.

I still do not know about the "should the inline be kept" question. The proposed log message for the commit does not explain (let alone justify) why "optimization" is unneeded for the fuctions in the first place, which does not help.