[pacman-dev] [PATCH 2/2] Overhaul archive fgets function

[Allan McRae]

On 24/12/10 14:18, Dan McGee wrote:
> On Thu, Dec 23, 2010 at 5:11 PM, Allan McRae<allan at archlinux.org>  wrote:
>> On 22/12/10 04:22, Dan McGee wrote:
>>>
>>> On Sun, Dec 19, 2010 at 11:32 AM, Xavier Chantry
>>> <chantry.xavier at gmail.com>    wrote:
>>>>
>>>> On Wed, Dec 15, 2010 at 7:48 AM, Dan McGee<dan at archlinux.org>    wrote:
>>>>>
>>>>> The old function was written in a time before we relied on it for nearly
>>>>> every operation. Since then, we have switched to the archive backend and
>>>>> now
>>>>> fast parsing is a big deal.
>>>>>
>>>>> The former function made a per-character call to the libarchive
>>>>> archive_read_data() function, which resulted in some 21 million calls in
>>>>> a
>>>>> typical "load all sync dbs" operation. If we instead do some buffering
>>>>> of
>>>>> our own and read the blocks directly, and then find our newlines from
>>>>> there,
>>>>> we can cut out the multiple layers of overhead and go from archive to
>>>>> parsed
>>>>> data much quicker.
>>>>>
>>>>> Both users of the former function are switched over to the new
>>>>> signature,
>>>>> made easier by the macros now in place in the sync backend parsing code.
>>>>>
>>>>> Performance: for a `pacman -Su` (no upgrades available),
>>>>> _alpm_archive_fgets() goes from being 29% of the total time to 12% The
>>>>> time
>>>>> spent on the libarchive function being called dropped from 24% to 6%.
>>>>>
>>>>> This pushes _alpm_pkg_find back to the title of slowest low-level
>>>>> function.
>>>>>
>>>>> Signed-off-by: Dan McGee<dan at archlinux.org>
>>>>> ---
>>>>>
>>>>> I wish this one was a bit easier to comprehend and grok, but the cost of
>>>>> performance here is a bit more complicated fgets function than the very
>>>>> simplistic one we had before.
>>>>>
>>>>> The new function revolves around the zero-copy, retrieve an entire block
>>>>> functinoality of libarchive. We create a structure to keep all the
>>>>> bookkeeping
>>>>> info we need around, including a buffer of our own where each line ends
>>>>> up and
>>>>> auto-resizes itself as we poll through an archive file entry, and also
>>>>> cleans
>>>>> itself up when the file is EOF. This means malloc/free is not necessary
>>>>> by the
>>>>> caller, which keeps most concerns in the fgets function.
>>>>>
>>>>> Please let me know if it is unclear how the new function works, so more
>>>>> comments can be put in there.
>>>>>
>>>>
>>>> Since we have just one line, I would not have bothered trying to alloc
>>>> exactly what we need. I would just have used a static or dynamic buf
>>>> of max line size, and be done with it.
>>>> But anyway you already implemented the more complex way and I don't
>>>> see a problem with that as long as its tested (especially the re-alloc
>>>> part) :)
>>>
>>> I'm not sure what you are saying you would do with a dynamic buffer-
>>> doesn't it make sense to pick the original size as whatever we get
>>> back first, rather than any other arbitrary size that we might have to
>>> resize anyway? And I've always hated static buffer sizes; see the
>>> PATH_MAX junk for more reason for that.
>>>
>>>> I could not spot a problem by simply reading the code.
>>>>
>>>> Another minor observation : in the !done case, the alloc block does a
>>>> strict allocation and thus does not take into account that we will
>>>> very likely have to extend it on the next iteration (if we did not
>>>> reach the last archive block).
>>>>
>>>> Finally can it actually happen with our sync archives which only has
>>>> very small files that a line is split across two blocks ?
>>>
>>> It can with the smoke002.py pactest I'll include that just blew a hole
>>> through my realloc code; necessary changes below. Pactest created
>>> packages usually produce a block size of 65536 or so.
>>>
>>> I also bumped the max line size to 512KB rather than PATH_MAX (4096
>>> bytes), so now the delayed allocation makes a bit more sense.
>>>
>>>> In any cases, this is another very welcome improvement.
>>>
>>> Thanks!
>>>
>>> -Dan
>>>
>>> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
>>> index 87880de..d34eab5 100644
>>> --- a/lib/libalpm/util.c
>>> +++ b/lib/libalpm/util.c
>>> @@ -841,6 +842,8 @@ int _alpm_archive_fgets(struct archive *a, struct
>>> archive_read_buffer *b)
>>>                          /* we've looked through the whole block but no
>>> newline, copy it */
>>>                          size_t len = b->block + b->block_size -
>>> b->block_offset;
>>>                          memcpy(b->line_offset, b->block_offset, len);
>>> +                       b->line_offset += len;
>>> +                       b->block_offset = i;
>>>                  }
>>>          }
>>>
>>>
>>
>>
>> Would this fix some sort of infinite loop in database reading?   I just
>> experienced pacman "freezing" with 100% CPU usage while reading the [extra]
>> database.   I figured out that I had not rebuilt my pacman-git package since
>> applying this diff on my working branch and with a rebuild, everything went
>> away.
>>
>> So I figure this was the fix...  but I want to make sure that it makes sense
>> to be the fix and I have not just ignored another issue that is no longer
>> obviously exposed.
>
> Probably- try with the new patch on my fgets-perf branch in my repo.
>

I should have been clearer.   I do not see the issue with the newest 
version of your patch.

This was mainly me confirming to myself that the changes made to that 
patch fixed the issue, which on further inspection appears to be true. 
So all is good.

Allan