Re: [PATCH 9/9] migration/ram: calculate un/encoded_size only whenneeded.

[maozy]

On 6/12/20 3:05 AM, Dr. David Alan Gilbert wrote:
> * Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote:
> > Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
> > ---
> >   migration/ram.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 41cc530d9d..ca20030b64 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -910,14 +910,15 @@ static void migration_update_rates(RAMState *rs, int64_t 
> > end_time)
> >           xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss 
> > -
> >               rs->xbzrle_cache_miss_prev) / page_count;
> >           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> > -        encoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
> > -                         TARGET_PAGE_SIZE;
> > -        encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
> >           if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
> >               xbzrle_counters.encoding_rate = 0;
> > -        } else if (!encoded_size) {
> > +        } else if (xbzrle_counters.bytes == rs->xbzrle_bytes_prev) {
>
> No, I don't think this change is worth it - this is really just the same
> as 'encoded_size', and then we may as well keep the two together.

ok, thanks, let's keep 'encode_size' here.

BTW, this change borrows from the behavior of comppressed:

...
        compressed_size = compression_counters.compressed_size -
                          rs->compressed_size_prev;
        if (compressed_size) {
            double uncompressed_size = (compression_counters.pages -
                                    rs->compress_pages_prev) * 
TARGET_PAGE_SIZE;

            /* Compression-Ratio = Uncompressed-size / Compressed-size */
            compression_counters.compression_rate =
                                        uncompressed_size / 
compressed_size;
...


It splits 'compressed_size' and 'uncompressed_size', and calculates
'uncompressed_size' only when needed. Although 'unencoded_size' is
calculated, it is not necessarily used. if you think this split is
unnecessary, just discard it, so do I need to drop this patch and
resend the v2?

Thanks,
Mao

> Dave
>
> >               xbzrle_counters.encoding_rate = UINT64_MAX;
> >           } else {
> > +            unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
> > +                             TARGET_PAGE_SIZE;
> > +            encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
> > +
> >               xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
> >           }
> >           rs->xbzrle_pages_prev = xbzrle_counters.pages;
> > --
> > 2.17.1
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK