Re: [boost] GIL io_new review

Subject: Re: [boost] GIL io_new review
From: Domagoj Saric (dsaritz_at_[hidden])
Date: 2010-12-11 10:18:54

Hi Lubomir, apologies in advance for not covering all the points...short on
time ;)

"Lubomir Bourdev" <lbourdev_at_[hidden]> wrote in message
news:286B66C8-D267-417C-9442-58E4A491EBFD_at_adobe.com...
> All we can hope for is push the boundary as much as we can, and leave
> the rest for the next update.

As is often the case...however there are more future-proof and less
future-proof designs/interfaces (judged by how much can you
change/add/remove without breaking compatibility and/or performance)...Which
I think is what you also stressed below:
> What is important in the first release is to get the design right.
> Once we have a solid foundation we can add a lot of functionality quickly
> and transparently.

> On the issue of using classes vs free functions, I don't think there is a
> huge difference either way. The the new_io code borrows the style from
> GIL, which uses free functions as algorithms (if you call I/O
> "algorithms").
> GIL in turn borrows this style from the STL. Using functions one can
> extend
> functionality with overloading (but there are ways to do this with classes
> too)
> and it is nicer to be able to read/write a file with one line, without
> having to
> first construct a reader/writer and then call it. But these are minor
> quibbles.

I understand you had limited time to go through all the, now really numerous
posts, but I think I've shown (in the pre-review discussion) that these are
not minor quibbles...The free function interface has issues both in terms of
performance and in terms flexibility/power of access to the backend that it
gives...So far shown attempts to fix those where all shifts towards public
classes/objects representing the image to be read/written (and/or further
increase in the number of parameters)...

Furthermore, the classes approach can also provide a oneliner interface as
io2 already does (as shown in the pre-review discussion) using static member
functions...it is provided for all backends in a single place by the CRTP
base class...so you can for example simply write libpng_image::read( ... )
or libtiff_image::write( ... ) which will construct the local reader/writer
object for you and do the rest of the required work....

> On the issue of performance, I don't think one test with one file format
> being
> 30% slower proves that there are serious performance issues. My guess is
> that
> Christian has been focusing on adding functionality and never got around
> to
> work on performance improvements, which usually means that we can get a
> lot of speedup by doing some simple optimizations in the inner loops.

Actually there were two file formats (JPEG and PNG) and the speed difference
was ~37.5% (the size difference was ~30%)...In fact simply looking
at/stepping through the implementation of io and io_new (which I've
elaborated in more detail previously) is enough to 'see' the inefficiency
w/o testing but I did the test anyway...

I simply cannot follow the logic of making a priori slow software (by
ignoring obvious inefficiencies) and then making it fast later (unless of
course for the later selling of some 'clever' corporate marketing fog which
obviously is not the case here)...
There would be no need for serious performance improvements if it weren't
for the, unfortunately so prevalent, 'habit'/'school of coding' that simply
by default uses STL containers/constructs for just about anything without
even blinking an eye...
And the injudicious use of STL containers coupled with the inherent template
bloat are the major reasons why io_new (and the current io) are slow (not
counting the issues inherent in the interface/design)...

In this context I actually do not blame Christian almost at all for this but
the 'climate' that fosters this "Way"...

It is no wonder that 'we live in the age' of several-megabyte-single-dialog
software if 'every' library author thinks it OK to add a few tens or even a
couple of hundreds of unnecessary kilobytes of code because it supposedly
will not be noticeable because of disk access or some similar 'by-the-book'
excuse (that is rarely actually tested)...

ps. Ideally, in most cases there should be no 'inner-loop' in GIL.IO (other
than those within the backend library itself)...

> On the issue of using/not using streams, I don't have a strong opinion,
> except that stream support for I/O was one of the highest requested
> functionalities in GIL's review. Domagoj, you should have been more
> vocal at that time. Christian simply responds to the wishlist of the
> Boost community. In general, I feel that performance might be a bit
> overhyped in recent discussions. The image I/O operations are often
> dominated by disk access and by compression, etc. I don't think
> avoiding a copy of the pixels would make much of a difference in the
> majority of uses. That said, we do want to make sure GIL I/O is as fast
> as it can be.

There are assumptions in this which you must/should test before you start
believing in them...I have already done numerous tests and analysis (not
only for GIL.IO) that prove those assumptions wrong...

That iostreams are evil is a simple _fact_ demonstrated countless times by
numerous people...And it seems there is no longer any point in arguing this
fact as it is simply ignored by people that seem to have some sort of a
priori trust in the STL (and you can't rationally argue with 'blind faith')
as if it is some 'holy artefact' of the C++ language when in fact it is
sometimes its stumbling block...

When I discuss/consider 'efficiency' I always consider both code speed and
size (as this also affects the speed even if sometimes not of the program
itself when tested/considered in isolation but of the whole system affected
by N such 'bloating decisions'...i.e. link the result of sizeof( Windows
Vista installation folder ) to its performance...)...In this light it is IMO
wrong for any library author to make any bloat-inducing decisions based on
assumptions of the type 'oh the OS call I make will always be slower than
anything I can possibly do'...By that rationale Boost.ASIO could then
allocate random std::strings before making a network call 'that will surely
be slower than anything done in a local process'...

With that said, I have no problem with providing an iostream interface (as
an additional option, that has no influence on other code) only with
providing _only_ an iostream interface (which AFAICT is currently the case
for in memory images...luckily not for all of io_new)...

> ...on the basis that there is another alternative libarary (Domagoj's
> library).
> I don't think this is a good idea. I looked briefly at his library; there
> are things
> I like about it but also things I don't. There are certainly a lot of
> things that
> are missing. I will refrain from discussing it because it is not under
> review

Of course, there are things I don't like in it :) As you said it is not
under review ;)

> This is a huge effort on behalf of Christian, he produced some great
> and very useful work and a reject will be demoralizing after so many
> years of work, and is unjustified given that fundamentally he has the
> right design.

Of course, I agree fully (except for the last part, that io_new has a
fundamentally right design)...
My vote 'no' was not a 'rejection' vote: as I've stated numerous times I
think Christian has done a truly massive piece of work...but...a massive
piece of work can also require a massive review process (as this is turning
out to be)...of which it can be expected to give a few 'unsatisfactory'
marks in the process before reaching a successful end...much like a mentor
might not simply reject a student's thesis but return it for a work up...

The 'massiveness' of the thing in our hands is further pointed out by the
fact that we have so far mostly or even exclusively seen reviews of only the
IO extension which comprises only a part of Christian's work (which I
suspect might also be 'frustrating' for Christian...as even a negative
review is often better than no review at all)...

> - Support for different backends. It appears that this can be done simply
> the
> same way a file format can be extended, i.e. have "png_libpng" and
> "png_lodepng"
> tags. It is not immediately obvious to me why this would be a bad design
> choice;
> if someone disagrees they should elaborate.

For example adding another set of tags could further complicate
things/implementation (even maybe slowing compilation) with another required
tag->implementation mapping...Directly using the backend wrapper (e.g.
libjpeg_image::... ) seems IMHO simpler with no adverse effects...

> Finally, here is an idea that I like, but may be too much work to do in
> the current release:
> I would like to have a GIL virtual image view associated with a given
> file. That would
> allow reading/writing to simply be copy_pixels, and reading into a
> different format to
> be copy_and_convert_pixels. Reading a given ROI could simply be done by
> creating a
> subimage_view. The cool thing about this is one could in theory make
> changes to
> images without even loading them in memory. The caviat is that doing this
> right and
> efficiently is very hard, requires clever caching, plus a wrong access
> pattern can
> result in horribly bad performance.

Actually this is what I tried to do at the very beginning when I started to
work on io2 with the GDI+ backend (the gp_view classes at the end of
gp_image.hpp, it is relatively easy to do this with GDI+ and WIC) but I
stopped after an initial prototype version as I of course had to catch up
and concentrate on a zilion other features either already present in io_new
or simply of a higher priority...

ps. There is also one important aspect (that has AFAICT only been briefly
mentioned so far) and that is of a header only library...having dealt with a
good number of different backends (and their various 'quirks') and being
able to extract/produce a fair amount of non-template code I'd also vote for
a change to a non-header-only library for various reasons...

-- 
"What Huxley teaches is that in the age of advanced technology, spiritual
devastation is more likely to come from an enemy with a smiling face than
from one whose countenance exudes suspicion and hate."
Neil Postman