[ovs-dev] [PATCH] OVN python IDL: avoid useless JSON conversion to enhance performance

[Lucas Alvares Gomes]

Hi,

Excellent investigative work here Daniel, thanks for that!

On Wed, Feb 28, 2018 at 9:37 AM, Miguel Angel Ajo Pelayo
<majopela at redhat.com> wrote:
> And if we can get backports down the lane, that'd be great :)
>

+1 for backporting it. The changes seems straight forward to do so.

> On Wed, Feb 28, 2018 at 9:37 AM Miguel Angel Ajo Pelayo <majopela at redhat.com>
> wrote:
>
>> Acked-by: Miguel Angel Ajo <majopela at redhat.com>
>>
>> On Wed, Feb 28, 2018 at 9:13 AM Daniel Alvarez Sanchez <
>> dalvarez at redhat.com> wrote:
>>
>>> Thanks Terry and Han for the reviews!
>>> I removed the 'OVN' keyword from the title and submitted a v2:
>>> [PATCH v2] python: avoid useless JSON conversion to enhance performance
>>>
>>> Thanks again.
>>> Daniel
>>>
>>> On Wed, Feb 28, 2018 at 12:41 AM, Han Zhou <zhouhan at gmail.com> wrote:
>>>
>>> > Great catch!
>>> > This patch is generic and not only for OVN, so I suggest to remove the
>>> > "OVN" keyword in commit title.
>>> >
>>> > Acked-by: Han Zhou <hzhou8 at ebay.com>
>>> >
>>> > On Tue, Feb 27, 2018 at 12:44 PM, Terry Wilson <twilson at redhat.com>
>>> wrote:
>>> >
>>> >> On Tue, Feb 27, 2018 at 9:25 AM, Daniel Alvarez <dalvarez at redhat.com>
>>> >> wrote:
>>> >> > This patch removes a useless conversion to/from JSON in the
>>> >> > processing of any 'modify' operations inside the process_update2
>>> >> > method in Python IDL implementation.
>>> >> >
>>> >> > Previous code will make resources creation take longer as the number
>>> >> > of elements in the row grows because of that JSON conversion. This
>>> >> > patch eliminates it and now the time remains consant regardless
>>> >> > of the database contents improving performance and scaling.
>>> >> >
>>> >> > Reported-by: Daniel Alvarez Sanchez <dalvarez at redhat.com>
>>> >> > Reported-at: https://mail.openvswitch.org/p
>>> >> ipermail/ovs-discuss/2018-February/046263.html
>>> >> > Signed-off-by: Daniel Alvarez <dalvarez at redhat.com>
>>> >> > ---
>>> >> >  python/ovs/db/idl.py | 12 +++++-------
>>> >> >  1 file changed, 5 insertions(+), 7 deletions(-)
>>> >> >
>>> >> > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
>>> >> > index 60548bcf5..5a4d129c0 100644
>>> >> > --- a/python/ovs/db/idl.py
>>> >> > +++ b/python/ovs/db/idl.py
>>> >> > @@ -518,10 +518,8 @@ class Idl(object):
>>> >> >              if not row:
>>> >> >                  raise error.Error('Modify non-existing row')
>>> >> >
>>> >> > -            old_row_diff_json = self.__apply_diff(table, row,
>>> >> > -
>>> row_update['modify'])
>>> >> > -            self.notify(ROW_UPDATE, row,
>>> >> > -                        Row.from_json(self, table, uuid,
>>> >> old_row_diff_json))
>>> >> > +            old_row = self.__apply_diff(table, row,
>>> >> row_update['modify'])
>>> >> > +            self.notify(ROW_UPDATE, row, Row(self, table, uuid,
>>> >> old_row))
>>> >> >              changed = True
>>> >> >          else:
>>> >> >              raise error.Error('<row-update> unknown operation',
>>> >> > @@ -584,7 +582,7 @@ class Idl(object):
>>> >> >                          row_update[column.name] =
>>> >> self.__column_name(column)
>>> >> >
>>> >> >      def __apply_diff(self, table, row, row_diff):
>>> >> > -        old_row_diff_json = {}
>>> >> > +        old_row = {}
>>> >> >          for column_name, datum_diff_json in six.iteritems(row_diff):
>>> >> >              column = table.columns.get(column_name)
>>> >> >              if not column:
>>> >> > @@ -601,12 +599,12 @@ class Idl(object):
>>> >> >                            % (column_name, table.name, e))
>>> >> >                  continue
>>> >> >
>>> >> > -            old_row_diff_json[column_name] =
>>> >> row._data[column_name].to_json()
>>> >> > +            old_row[column_name] = row._data[column_name].copy()
>>> >> >              datum = row._data[column_name].diff(datum_diff)
>>> >> >              if datum != row._data[column_name]:
>>> >> >                  row._data[column_name] = datum
>>> >> >
>>> >> > -        return old_row_diff_json
>>> >> > +        return old_row
>>> >> >
>>> >> >      def __row_update(self, table, row, row_json):
>>> >> >          changed = False
>>> >> > --
>>> >> > 2.13.5
>>> >> >
>>> >> > _______________________________________________
>>> >> > dev mailing list
>>> >> > dev at openvswitch.org
>>> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> >>
>>> >> +1
>>> >>
>>> >> This passes all of the functional tests in ovsdbapp when applied. Nice
>>> >> find!
>>> >>
>>> >> Terry
>>> >> _______________________________________________
>>> >> dev mailing list
>>> >> dev at openvswitch.org
>>> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> >>
>>> >
>>> >
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev