Pimpl-ization

classic Classic list List threaded Threaded
11 messages Options
Kohei Yoshida-7 Kohei Yoshida-7
Reply | Threaded
Open this post in threaded view
|

Pimpl-ization

It has come to my attention that some folks are feeling a bit uneasy
about my recent pimplization of some of what I call "high impact" public
classes.  I just wanted to make it clear that the reason I'm doing this
is in direct reponse to the discussion we recently had during our ESC
call wrt the bloated object sizes of our shared objects, most notably
those generated from sc and sw modules.

There are 2 main reasons I'm doing this.

1) To reduce the size of our shared objects.
2) To improve our build time.

1) is apparently important for Michael Stahl, and I personally care
about 2) since it directly affects my development efficiency.

My belief was that this was not a controvercial change, but it appears
that it is.  If there is a strong objection I'll stop doing it.  If not,
I'd like to continue pursing this since it seems to improve both of the
2 aforementioned points, and I've only covered perhaps 10-20% of all
possible candidates.

Please share your opinions.

Kohei

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Miklos Vajna-4 Miklos Vajna-4
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

Hi Kohei,

On Wed, Dec 10, 2014 at 09:09:04PM -0500, Kohei Yoshida <[hidden email]> wrote:

> 1) To reduce the size of our shared objects.
> 2) To improve our build time.
>
> 1) is apparently important for Michael Stahl, and I personally care
> about 2) since it directly affects my development efficiency.
>
> My belief was that this was not a controvercial change, but it appears
> that it is.  If there is a strong objection I'll stop doing it.  If not,
> I'd like to continue pursing this since it seems to improve both of the
> 2 aforementioned points, and I've only covered perhaps 10-20% of all
> possible candidates.
>
> Please share your opinions.
I very much appreciate the work you do here, if regular developers
ignore the build time issues with buying faster and faster HW, then at
some point we won't be able to attract new contributors.

I know that some feel this makes debugging harder, as printing the
object will now show only a pImpl pointer and not the data members, but
then gdb can call method calls just fine, so calling the relevant
members isn't that hard, IMHO. (Or if it's a frequently used object, a
pretty-printer can be added.)

Thanks,

Miklos

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

signature.asc (188 bytes) Download Attachment
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

In reply to this post by Kohei Yoshida-7
On 12/11/2014 03:09 AM, Kohei Yoshida wrote:
> Please share your opinions.

I think, as always, the pros and cons have to be weighed.

The benefit is decoupling.  It apparently has a positive impact on the
size of debug information put into link objects by today's toolchains.
(And I would be surprised if it had an impact on any non-debug parts of
link objects.)  Another positive impact is improved build time with
today's toolchains (sans pch).  (But remember that pimpl-ization will
always need to include <memory> into the include file, so don't go
overboard with erasing standard library includes from include files.)

Disadvantages that come to mind are increased usage of dynamic memory
(which can impact both space and time), and, to some extend at least,
increased complexity of the code "in the small" (although complexity "in
the large" is probably reduced by the decoupling).
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Jan Holesovsky-4 Jan Holesovsky-4
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

In reply to this post by Kohei Yoshida-7
Hi Kohei,

Kohei Yoshida píše v St 10. 12. 2014 v 21:09 -0500:

> My belief was that this was not a controvercial change, but it appears
> that it is.  If there is a strong objection I'll stop doing it.  If not,
> I'd like to continue pursing this since it seems to improve both of the
> 2 aforementioned points, and I've only covered perhaps 10-20% of all
> possible candidates.

I don't have objection if _you_ are doing that :-)  It's fine for me
when it is targeted, focused on the goals you've outlined.

But I did not know the reasons for pimpl-ization previously, and my fear
was that this might become a favorite Easy Hack where we'd have loads of
people pimpl-izing just everything; which was my concern from 2 reasons:

* debuggability / code reading perspective: one more level of
  indirection that you need to go through to see what's going on

* cost of new/delete of the Impl class + cost of the pImpl-> calls

If it is not going to become such an Easy Hack, I am OK with that of
course.

All the best,
Kendy

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Wols Lists Wols Lists
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

In reply to this post by Miklos Vajna-4
On 11/12/14 08:06, Miklos Vajna wrote:
> I very much appreciate the work you do here, if regular developers
> ignore the build time issues with buying faster and faster HW, then
> at some point we won't be able to attract new contributors.

As a gentoo-er, (and there are other similar distros :-) this also
impacts directly on the end user.

(I think it's down to slightly dodgy hardware, a self-build :-) but I
have great difficulty building the gentoo LO emake)

Cheers,
Wol
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Tor Lillqvist-2 Tor Lillqvist-2
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

In reply to this post by Kohei Yoshida-7

Please share your opinions.

Well, I guess this email was caused by some complaints by me on IRC some days ago... It was just that, complaints on IRC. Don't take it too seriously. There will always be some workaround when debugging...

--tml


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Thorsten Behrens Thorsten Behrens
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

In reply to this post by Kohei Yoshida-7
Kohei Yoshida wrote:
> 1) To reduce the size of our shared objects.
>
The above should not be underestimated for debug builds - there's
still lots of laptop hardware in student hands that has a physical
limit on RAM size (e.g. 4GB). I'd hate to have hacking LibO becoming
an absolutely unbearable experience for them.

Cheers,

-- Thorsten

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

signature.asc (985 bytes) Download Attachment
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

In reply to this post by Kohei Yoshida-7
Hi,

On Wed, Dec 10, 2014 at 09:09:04PM -0500, Kohei Yoshida wrote:
> 1) To reduce the size of our shared objects.
> 2) To improve our build time.
> [...]
> Please share your opinions.

Both are valuable goals and doers are deciders, so sounds good to me.

As a note though, I personally prefer an abstract base class plus factory
function over pimpl though. For me, it removes boilerplate (and leaves it for
the compiler to handle) and is more readable:

Pimpl

(HXX):

class AnotherThing;
class RealThing;

struct Thing {
    Thing(AnotherThing* pAnother);
    void DoSomething()
        { m_pImpl->DoSomething() }
    void DoSomethingElse()
        { m_pImpl->DoSomethingElse() }
    ...
    std::unique_ptr<RealThing> m_pImpl;
}

(CXX):

struct RealThing SAL_FINAL {
    RealThing(AnotherThing* pAnother)
        : m_pAnother(pAnother) {}
    void DoSomething();
    void DoSomethingElse();
    AnotherThing* m_pAnother;    
}

Thing::Thing(AnotherThing* pAnother)
    : m_pImpl(new(RealThing(pAnother)))
{}

RealThing::DoSomething()
{ .... }
RealThing::DoSomethingElse()
{ .... }

ABC plus factory function

(HXX):

class AnotherThing;
struct Thing {
    static Thing* Create(AnotherThing* pAnother);
    virtual void DoSomething()=0;
    virtual void DoSomethingElse()=0;
    ...
    virtual ~Thing() {};
}


(CXX):

struct RealThing SAL_FINAL {
    RealThing(AnotherThing* pAnother)
        : m_pAnother(pAnother) {}
    virtual void DoSomething();
    virtual void DoSomethingElse();
    AnotherThing* m_pAnother;
}

Thing* Thing::Create(AnotherThing* pAnother)
    { return new(RealThing(pAnother)); }

RealThing::DoSomething()
{ .... }
RealThing::DoSomethingElse()
{ .... }

Both of these do one heap alloc. Cosmetically the ABT is already shorter
LOC-wise, Pimpl needs more boilerplate for each added member function and the
ABC looks much better in e.g. doxygen. Its true that you can use the Pimpl
Thing-class as a "value type" and not the ABC, but the former is just a wrapper
around a pointer anyway, so this isnt of much practical relevance AFAICS.

To repeat: Im fine with adding Pimpls were sensible. But Id like to suggest to
maybe also consider ABCs were sensible.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

On 12/11/2014 01:49 PM, Bjoern Michaelsen wrote:
> To repeat: Im fine with adding Pimpls were sensible. But Id like to suggest to
> maybe also consider ABCs were sensible.

...or KISS and have a plain struct pimpl

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Stahl-2 Michael Stahl-2
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

In reply to this post by Bjoern Michaelsen
On 11.12.2014 13:49, Bjoern Michaelsen wrote:

> Pimpl
>
> (HXX):
>
> class AnotherThing;
> class RealThing;
>
> struct Thing {
>     Thing(AnotherThing* pAnother);
>     void DoSomething()
>         { m_pImpl->DoSomething() }
>     void DoSomethingElse()
>         { m_pImpl->DoSomethingElse() }
>     ...
>     std::unique_ptr<RealThing> m_pImpl;
> }
>
> (CXX):
>
> struct RealThing SAL_FINAL {
>     RealThing(AnotherThing* pAnother)
>         : m_pAnother(pAnother) {}
>     void DoSomething();
>     void DoSomethingElse();
>     AnotherThing* m_pAnother;    
> }
>
> Thing::Thing(AnotherThing* pAnother)
>     : m_pImpl(new(RealThing(pAnother)))
> {}
>
> RealThing::DoSomething()
> { .... }
> RealThing::DoSomethingElse()
> { .... }

forward every single method to a method of the pimpl class?  i've never
seen anybody do that; usually the pimpl class only has public members
and "private" methods (that shouldn't be mentioned in the header file
anyway) which is a bit simpler than your straw-man.


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Pimpl-ization

On Thu, Dec 11, 2014 at 04:01:04PM +0100, Michael Stahl wrote:
> forward every single method to a method of the pimpl class?  i've never
> seen anybody do that

Oh, I did see that done a lot. Anyway, even without that, speed is the same
with -O3 and an abstract base class is still shorter. But as said: Im not
really offended by pImpls so lets stop here.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice