[RFC,08/21] qapi: Touch generated files only when they change

A massive number of objects depends on QAPI-generated headers.  In my
"build everything" tree, it's roughly 4500 out of 4800.  This is
particularly annoying when only some of the generated files change,
say for a doc fix.

Improve qapi-gen.py to touch its output files only if they actually
change.  Rebuild time for a QAPI doc fix drops from many minutes to a
few seconds.  Rebuilds get faster for certain code changes, too.  For
instance, adding a simple QMP event now recompiles less than 200
instead of 4500 objects.  But adding a QAPI type is as bad as ever; we
clearly got more work to do.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

On 02/02/2018 07:03 AM, Markus Armbruster wrote:
> A massive number of objects depends on QAPI-generated headers.  In my
> "build everything" tree, it's roughly 4500 out of 4800.  This is
> particularly annoying when only some of the generated files change,
> say for a doc fix.
> 
> Improve qapi-gen.py to touch its output files only if they actually
> change.  Rebuild time for a QAPI doc fix drops from many minutes to a
> few seconds.  Rebuilds get faster for certain code changes, too.  For
> instance, adding a simple QMP event now recompiles less than 200
> instead of 4500 objects.  But adding a QAPI type is as bad as ever; we
> clearly got more work to do.

The last phrase sounds quite colloquial.  It may have been intentional;
but if not, s/we/we've/

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi/common.py | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index cfa2671ca3..be0fcc548a 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1944,9 +1944,16 @@ class QAPIGen(object):
>              except os.error as e:
>                  if e.errno != errno.EEXIST:
>                      raise
> -        f = open(os.path.join(output_dir, fname), 'w')
> -        f.write(self.top(fname) + self._preamble + self._body
> +        fd = os.open(os.path.join(output_dir, fname),
> +                     os.O_RDWR | os.O_CREAT, 0666)
> +        f = os.fdopen(fd, 'r+')
> +        text = (self.top(fname) + self._preamble + self._body
>                  + self.bottom(fname))
> +        oldtext = f.read(len(text) + 1)
> +        if text != oldtext:
> +            f.seek(0)
> +            f.truncate(0)
> +            f.write(text)

In-memory rewrite rather than playing games with a secondary file
combined with the rename(2) syscall.  It means you're not atomic (an
external reader has a window of time where they can see an incomplete
version of the file).  But I think make dependency rules mean we don't
care - even in a parallel rule, as long as all clients depend on the
timestamp file, and the timestamp file isn't updated until all our
rewrites (if any) have settled, then we don't create any of those
external readers that can find the window of incomplete contents.

Reviewed-by: Eric Blake <eblake@redhat.com>

Eric Blake <eblake@redhat.com> writes:

> On 02/02/2018 07:03 AM, Markus Armbruster wrote:
>> A massive number of objects depends on QAPI-generated headers.  In my
>> "build everything" tree, it's roughly 4500 out of 4800.  This is
>> particularly annoying when only some of the generated files change,
>> say for a doc fix.
>> 
>> Improve qapi-gen.py to touch its output files only if they actually
>> change.  Rebuild time for a QAPI doc fix drops from many minutes to a
>> few seconds.  Rebuilds get faster for certain code changes, too.  For
>> instance, adding a simple QMP event now recompiles less than 200
>> instead of 4500 objects.  But adding a QAPI type is as bad as ever; we
>> clearly got more work to do.
>
> The last phrase sounds quite colloquial.  It may have been intentional;
> but if not, s/we/we've/

I'll change it.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi/common.py | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index cfa2671ca3..be0fcc548a 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -1944,9 +1944,16 @@ class QAPIGen(object):
>>              except os.error as e:
>>                  if e.errno != errno.EEXIST:
>>                      raise
>> -        f = open(os.path.join(output_dir, fname), 'w')
>> -        f.write(self.top(fname) + self._preamble + self._body
>> +        fd = os.open(os.path.join(output_dir, fname),
>> +                     os.O_RDWR | os.O_CREAT, 0666)
>> +        f = os.fdopen(fd, 'r+')
>> +        text = (self.top(fname) + self._preamble + self._body
>>                  + self.bottom(fname))
>> +        oldtext = f.read(len(text) + 1)
>> +        if text != oldtext:
>> +            f.seek(0)
>> +            f.truncate(0)
>> +            f.write(text)
>
> In-memory rewrite rather than playing games with a secondary file
> combined with the rename(2) syscall.  It means you're not atomic (an
> external reader has a window of time where they can see an incomplete
> version of the file).

Same as before.

>                        But I think make dependency rules mean we don't
> care - even in a parallel rule, as long as all clients depend on the
> timestamp file, and the timestamp file isn't updated until all our
> rewrites (if any) have settled, then we don't create any of those
> external readers that can find the window of incomplete contents.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!