On 09/28/2015 05:27 AM, Daniel P. Berrange wrote:
On Fri, Sep 25, 2015 at 12:41:23PM -0700, David Lutterkort wrote:
> On Fri, Sep 25, 2015 at 11:35 AM, Laine Stump <laine(a)laine.org> wrote:
>
>> On 09/25/2015 01:27 PM, Daniel P. Berrange wrote:
>>
>>> So, I instrumented the netcf and augeas code to checking timings.
>>>
>>> The aug_get calls time less than a millisecond, as do the various
>>> other calls. I found the bulk of the time is actually coming from
>>> the netcf function "get_augeas", which in turns call
"aug_load"
>>> for every single damn netcf function call. So when we have 500
>>> interfaces, we're telling augeas to load all the config files
>>> 1000 times. That's where the slowness is coming from....
>>>
>>> Either we need to stop loading config files on every fnuction
>>> call in netcf,
>>>
>> Right you are! netcf has a variable "load_augeas" that is set each time
an
>> API call is started. I removed the line that sets it, and the time for
>> virsh iface-list --all for 309 toplevel interfaces (300 vlans connected to
>> 300 bridges + whatever is really on the host) went from 22.2 sec. to 1.47
>> sec!
>>
> Strange .. that variable should just control whether we call aug_load or
> not, it shouldn't affect whether we actual create a new augeas instance
> (which indeed can be very expensive)
>
> I see that we pass AUG_NO_MODL_AUTOLOAD to aug_init - it should not be
> necessary, but you might want to try quickly if passing
> AUG_NO_MODL_AUTOLOAD|AUG_NO_LOAD changes things (it shouldn't)
>
>
>> But of course we can't blindly re-use the initial data forever. Looking at
>> aug_load() itself, I see a lot of comments about everything it does to
>> avoid unnecessary re-loading of files. This was added in 2010, and is
>> probably what I remember David talking about:
>>
>> commit 5ee8163051be8214507c13c86171ac90ca7cb91f
>> Author: David Lutterkort <lutter(a)redhat.com>
>> Date: Tue Jun 29 15:32:44 2010 -0700
>>
>> Avoid unnecessary file parsing when reloading the tree
>>
>> We used to reparse every file we knew about upon aug_load. Now, we only
>> reparse files if the file has changed on disk.
>>
>> We test a few scenarios to make sure aug_load retains its behavior of
>> obliterating the tree and filling it with the latest from disk. This
>> includes throwing away unsaved changes or trees that have been deleted.
>>
>> So now the question is whether something can be done to improve aug_load()
>> (maybe it used to perform better and there has been a regression?), or if
>> netcf should derive a list of files from the list sent to augeas, and do
>> its own checking of the timestamps.
>>
> What should happen when you call aug_load is that augeas goes and stats all
> the files it knows about and if their mtime hasn't changed since we last
> loaded them, that should be the end of it. I have a hard time believing
> that stating 309 files takes ~ 1.5s though - seems there's something else
> going on. Maybe there are still some cases in which a new augeas instance
> gets created ?
Oh, you are mis-reading the numbers that Laine gives here. He has 310
interfaces, which corresponds to 620 API calls into netcf, which is
620 cals to aug_load() in 22 seconds. So each aug_load() is 35 milliseconds.
I've just tested on my machine with SSD and 500 interfaces. I get 25 ms
for each call to aug_load().
strace'ing it, I confirm that is really is only doing stat() on a whole
bunch of files.
So really the problem we have here is that we are doing O(n^2) calls to
aug_load() where n == number of interfaces.
So we really must change what we do so that we only do 1 call to aug_load
in order to list interfaces. I'm thinking the simplest way is to just
define a new API in augeas to bulk query the interfaces + their macs
in a single call.
I think you mean a new netcf API :-)
How about this as an alternative: the "load_augeas" flag in netcf could
instead be a timestamp, and we would only reload the augeas tree if it
had been at least [some very small number still much greater than
35msec] had passed since the last time it was loaded.
Doing it that way we could avoid adding complexity to the API for a
single use case, and we would also be able to easily backport it to
downstream releases of earlier versions of netcf (e.g. RHEL/CentOS 6)
(BTW, thanks for all the detective work Dan!)