Question about the interface classes

[Håvard Wall]

On Tuesday 26 June 2007 15:11, Matthias Kretz wrote:
[...]
> >   This reduced the library size with 32KB on x86 and 16KB on ARM.
>
> Interesting. OTOH there will be another saving in the backend which doesn't
> have to make all functions slots.

That's true! -)

BTW: to be fair I should mention that my ARM toolchain does not support symbol 
visibility. I assume this will explain some of the large differences in 
library size between x86 and arm. I'll try to get a newer toolchain to see 
how big difference this makes.

> > I agree this is not a huge reduction, and I'll be happy if we find other
> > ways of cutting down the library overhead instead.
>
> Do you have an idea how to profile this? Disassembling the stripped lib and
> see how big the functions are?

I guess that's the most exact way of doing it. Currently I've only been 
looking roughly at the (unstripped) object-files hoping that this gives an 
indication on which classes are the largest. Some scripts around readelf(1) 
might also be useful.

> > From an embedded
> > perspective I still think it's worthwile at least considering if some of
> > the functions could be moved into the interface classes. In addition to
> > the reduced overhead of the actual function, such a size reduction is
> > likely to improve cache behavior as well.
>
> You mean invokeMethod -> virtual function with moving? I agree, of course,
> that invokeMethod is the most expensive thing to do...

Yes, I guess my patch was the extreme. If you don't think it's the right thing 
to do, we'll leave it for now. If there's a subset of the invokeMethods that 
you think makes sense to make as virtual functions, that fine too.

> > FYI, I've attached the patch I used to do the size measurements. This is
> > not a final patch I suggest being commited, it would need some clean-ups
> > first.
>
> You forgot to "svn add" the new interface headers. 

Bah, I forgot that. Stupid me. Attached the missing files.

> While looking at it, do 
> you think we should remove
> bool supportsVideo()
> bool supportsOSD()
> bool supportsSubtitles()

How about making these functions inline and relying on a single getter in the 
backend? Something like this maybe:

class BackendCapabilities
{
public:
    enum CapabilitiesFlag { // probably a bad name
        Audio = 0x1,
        Video = 0x2,
        Subtitles = 0x4,
        ...
    };
    Q_DECLARE_FLAGS(CapabilitiesFlags, CapabilityFlag)

    inline bool supportsVideo() const { return capabilities() & Video; }

private:
    CapabilitiesFlags capabilities() const;   // probably bad naming
};
Q_DECLARE_OPERATORS_FOR_FLAGS(BackendCapabilities::CapabilitiesFlags)

class BackendInterface
{
public:
     virtual BackendCapabilities capabilities() const = 0;
};

Not sure if it's a bad idea. Only works for boolean capabilities, and might 
not be consistent with the rest of the API. Currently doesn't reduce much 
size, but it's easy to add support for new supportsFeatureX() in the future 
without breaking binary compatibility (and without increasing the library 
size -).

> ? At least OSD currently is not in the public API and should really not be
> there. Whether subtitles are optional I don't know, is video support
> optional for a backend?

Since Phonon allows you to play an audio file only, my personal opinion is 
that it makes sense to make video support optional. (I even think audio 
support could be optional, you might have a system with a screen but no audio 
output.)

--
hw
-------------- next part --------------
A non-text attachment was scrubbed...
Name: interfacevirtuals_missingfiles.patch
Type: text/x-diff
Size: 8817 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/phonon-backends/attachments/20070626/d930c991/attachment-0001.bin