Copy-by-value in sdbus++ generated objects

[Patrick Venture]

On Mon, Nov 5, 2018 at 7:47 AM Patrick Venture <venture at google.com> wrote:
>
> On Sun, Nov 4, 2018 at 6:36 PM Lei YU <mine260309 at gmail.com> wrote:
> >
> > On Sat, Nov 3, 2018 at 12:33 AM Patrick Venture <venture at google.com> wrote:
> > >
> > > Most repos inherit from an auto-generated object.  Those objects pass
> > > all parameters by value.  This is fine for many cases where they would
> > > need to create a copy of it, but in the case where:
> > >
> > > for instance:
> > > https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/templates/property.mako.prototype.hpp.in#L69
> > >
> > > The values match it's wasteful to copy.  It's also possible the cpp I
> > > learned way back is different in how this behavior works, but I wanted
> > > to check.  Because of the prevalence of this code, if there are lots
> > > of updates that don't actually change the value, it could optimize
> > > quite a bit at run-time.
> >
> > Agreed, this piece of code does not change the value at all, so it is better
> > to use const reference.
> >
> > >
> > > It would require a lot of simultaneous fixes (or two different objects
> > > generated, and transitioning everything).
> > >
> > > Thoughts?
> >
> > Maybe we could try to change this to const reference, do a build and see if it
> > cause any build error.
> > I guess this is OK.
>
> So, I can guess without building that this change will generate a lot
> of errors as all the overriding objects will be set to "override" but
> not be overriding anything.  So, compilations will fail pretty much
> across the board.  To be fair, it's actually maybe 20 repos that would
> need fixing, so not that big of an order, but yeah, I don't think this
> change can go in without a bunch of other simultaneous changes (unless
> we add something like a suffix, '_next" or "_new_" or "_2" and then
> slowly move things to calling that and then when everything is calling
> that, make the original the same, and then move everyone onto the
> original one.

I was able to make the change and the build revealed the objects (as
you would expect) that need updating and it occurred to me that
although locked srcrev bumps would solve the incompatibility issue, it
would make more sense to do the suffixed version of the method and
then transition all the repos and then transition them again so that
at each point all the CIs would work and all the versions would work.
After fixing up a few repos already, I can tell it's not that much
work and in the equal value case, at least, it'll improve the
performance of code that is pervasive throughout openbmc.

Patrick