Re: Some rough edges of core.fsmonitor

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 1, 2018, 10:40 UTC
Message-ID: <CACsJy8CShVSQAZ6ZaGT9i0JDEn19cTMaEmLRDV3ULjaj9DqZUg@mail.gmail.com>
In-Reply-To: <bb8a433f-bd61-1f3c-2034-1acc96539882@gmail.com>

On Tue, Jan 30, 2018 at 6:16 AM, Ben Peart <peartben@gmail.com> wrote:

Show 28 quoted lines

>
>
> On 1/29/2018 4:40 AM, Duy Nguyen wrote:
>>
>> On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> b) with fsmonitor
>>>
>>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>>>      12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s:
>>> read cache .git/index
>>
>>
>> This is sort of off topic but may be interesting for big repo guys. It
>> looks like read cache's time is partially dominated by malloc().
>>
>
> That is correct.  We have tried a few different ways to address this. First
> was my patch series [1] that would parallelize all of the read cache code.
>
> We quickly found that malloc() was the biggest culprit and by speeding that
> up, we got most of the wins.  At Peff's recommendation [2], we looked into
> using tcmalloc but found that 1) it has bugs on Windows and 2) it isn't
> being actively maintained so it didn't seem those bugs would ever get fixed.
>
> We are currently working on a patch that will use a refactored version of
> the mem_pool in fast-import.c to do block allocations of the cache entries
> which is giving us about a 22% improvement in "git status" times.  One

My apologies if this has been discussed in the second half of 2017 which I have no idea what happened.

I just wonder if it's possible to design a "file format" that is basically a memory dump of lots of struct cache_entry? This "file" will stay in a shared memory somewhere and never get written down to disk. Since it's very close to the data structure we have in core, the most we need to do after mmap'ing it (and keeping it mmap'd until the end) is adjust some pointers. These entries are of course read-only. When you modify/create new entries, they are created new, using the old malloc(). We just need to make sure not free the read-only cache_entry entries and munmap() the whole thing when we discard_index().

This opens up another option to deal with the large UNTR and TREE extensions in a similar way. These will be your next headache after you have reduced parse time for main entries.

> challenge has been ensuring that cache entries are not passed from one
> index/mem_pool to another which could cause access after free bugs.

We kind of have something close to that, but not entirely the same. When split index is used, the same cache_entry can appear in two index_state structs. Of course you can free only one of them (and you can only do so when you know both index_state are gone). I see some code cleanup opportunity :)

Show 5 quoted lines

>
> [1]
> https://public-inbox.org/git/20171109141737.47976-1-benpeart@microsoft.com/
> [2]
> https://public-inbox.org/git/20171120153846.v5b7ho42yzrznqoh@sigill.intra.peff.net/

-- 
Duy