[GIL] new IO extension review

Hi everybody, this is my first official Boost review so <insert the usual disclaimer> ;) I'm glad to see that at last more people have joined the discussion... In case some have not noticed, there was a very recent 'pre-discussion' about io_new titled "[gil] New IO release" and can be found here: http://lists.boost.org/Archives/boost/2010/10/index.php http://lists.boost.org/Archives/boost/2010/10/171450.php ...where I've already written (about) my thoughts and objections in detail: http://lists.boost.org/Archives/boost/2010/10/171732.php For the sake of brevity (and tight time management :) I will not repeat all the details here, rather try to reiterate through them briefly (omitting tests, examples and/or detailed argumentation) while perhaps adding some new thoughts... Mateusz, can you please confirm whether this approach (to assume that the "[gil] New IO release" notes and conclusions are part of the final review discussion) is OK or should I somehow repack that discussion into the new one..? ------ What is your evaluation of the design? ------ While the design can be considered an improvement from GIL.IO I nonetheless consider it lacking. As the new interface already constitutes a breaking change, this opportunity could have been used to provide a much more powerful and flexible interface (while still maintaining ease of use). A brief reiteration from "[gil] New IO release", the free function interface is inflexible/weak because it: - makes it difficult or practically impossible to reuse underlying backend instances across calls which, for example, has serious performance implications for ROI based access or when you want to read the image info/metadata before reading the image... - (as is done now) couples the choice of the backend with the choice of the image format (i.e. it makes it much more difficult to use/add alternate backend implementations for various image formats, for example if one wants/has to use LodePNG or GDI+ or ... as a backend) - does not allow access to the underlying backend which is necessary both for faster/easier solving of real world problems (either later found deficiencies in the interface or more complex/unconventional/corner use cases) and a more future-proof design (that can be more easily extended without breaking changes and/or introduction of additional overheads) ------ What is your evaluation of the implementation? ------ Considering that the GIL.IO implementation already has an inefficient implementation (which was even hinted by the "this will be simplified" comments in the code) I would hope io_new to provide a step forward in this regard...unfortunately it turns out it is a big step backward, both in terms of code bloat and code speed (as demonstrated by the tests and in-depth analysis in the "[gil] New IO release" thread)... In fact, the mentioned test practically demonstrates a 'theoretical' effect: on an Intel i5 CPU, with a relatively large cache, io and io2 benefit from being compiled/optimized for speed (as opposed for size) while io_new actually executes slower when compiled for speed (probably because the inherent code size increase/excessive inlining which overflows even a large CPU cache)...this effect was not demonstrated when the same test was run a much weaker Via C7-M CPU (with a much smaller cache)... In addition to efficiency concerns, the io_new implementation (or 'internal design') is also inflexible in certain areas where it causes duplication/repetitive code between different backend wrappers (thus not making it easier to extend with new backends). ------ What is your evaluation of the documentation? ------ Did not build it... ------ What is your evaluation of the potential usefulness of the extensions? ------ A makeover for GIL.IO is long over due and the general ideas that Christian put in io_new are mostly very welcome even though I have serious objections as to how those ideas were actually put in practice... ------ Did you try to use the extensions? ------ Yes, but only JPEG, PNG and TIFF IO extensions (did not try the opencv, sdl and toolbox extensions)... ------ With what compiler? ------ MSVC++ 10.0 ------ Did you have any problems? ------ Yes, for example the broken TIFF reading-with-conversion, Christian did fix it later but the fix seems more like a patch as it demonstrates an inherent inflexibility (mentioned at the end of the implementation section) in the io_new design that requires the backend-wrapper maintainer to provide the (possibly very long) conversion switch cases for each backend... ------ How much effort did you put into your evaluation? ------ As mentioned, I dug only through the core IO extension (and specifically through the LibJPEG, LibPNG and LibTIFF backend wrappers) but the areas I did investigate I examined relatively thoroughly (and over a longer period of time...in parallel, over the many months of development of io2)... ------ Are you knowledgeable about the problem domain? ------ My main use case consists of simple image loading on application startup (for the application skin) and only minor adjustments (like PC to Mac gamma fixup). However, as I was unsatisfied with existing solutions (including the direction in which io_new went) I decided to roll my own solution and for this to get accepted I had to look into and cover all reasonably possible use cases (which included going through the GIL.IO documentation as well as old/previous GIL.IO related discussions). ------ Do you think the extensions should be accepted as a part of Boost.GIL library? ------ (To make it clear) The way they are now - no. However, as mentioned earlier, a makeover of GIL.IO is undoubtedly needed and the work Christian has done so far is truly massive. For example, the properties system could prove to be quite useful (as a C++ abstraction for the various options and metadata specific to each image format) if done right (if not coupled to a particular backend, so that, for example, the PNG properties can equally be used, if supported, with LibPNG, LodePNG, GDI+, WIC ....)... Actually, IMO some of the extensions (like new GIL image/pixel formats) should maybe enter GIL itself, not merely as extensions (this would possibly even make it easier to implement the more complex IO backends/formats, like TIFF, that support those types of images)... -- "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

Hi Domagoj, thanks for your lengthy review and your valuable insight.
I think it's good enough. You made you point very clear. ;-) We should definitely "sit down" after the review and see what should be the next step. I hope you're committed to walk the next few miles.
This review has shown me what some real world use cases would look like. I agree there are some short comings. I think in the end I was more committed into getting more image formats read, like bit_aligned images which I find a very important use case. But your point is and has been well taken.
Mhmm, not sure what to take from this comment. When do you consider inlining appropriate and when not. As far as I know the Spirit lib practically lives from inlining. Are you saying they overdo it, as well. I don't where to draw the line and rather have the compiler do its work.
I try to reuse code that can be shared between backends. I don't understand your point. Can you point out what is inflexible?
I think this is the first positive comment. ;-)
Back to negativity. Yes there was a bug back in September. I fixed it, not much more I can do about that. I hope you realize that providing a IO extension is more of a can of worms. Apart from all the different use cases, each image type comes with a lot of baggage and fine tuning potentials. No to mention all the supported formats. TIFF is the best and worst of both worlds. ;-) There are over 400 tests in my proposal and it's still not enough.
I'm not sure if sharing properties across backends makes sense. They all provide their own set. Even if they provide some matching properties ( like image height ) these properties might have different data types.
I actually think GIL should move specific pixel types into a separate extension. I understand GIL as some sort of meta-library without the knowledge of rgb or cymk. Thanks again for your long review. I think I lost some nerves along the way but I very much appreciate your efforts. Christian

"Christian Henning" <chhenning@gmail.com> wrote in message news:AANLkTinMFAmw77CL=7LP86o2RjKJ4a76E1i3Kcv9KRvb@mail.gmail.com...
We should definitely "sit down" after the review and see what should be the next step. I hope you're committed to walk the next few miles.
What's a few more miles ;)
Of course, ability to read for example 1 bit images w/o them taking about 8 times the required RAM is a valuable addition (if I assume correctly what your bit_aligned image can do), which, as mentioned earlier should probably enter into the core GIL.
And that (inlining done by the compiler) is what I was referring to...when you compile for speed the compiler will inline more functions, and without profile guided optimizations and precise information on the target CPU it cannot know what will actually be the optimum amount of inlining...thus resulting in some cases in the reverse effect because the extra generated code blows the instruction cache...And 'unnecessarily bloated' code is more prone to this effect than 'lean written' code...
I try to reuse code that can be shared between backends. I don't understand your point. Can you point out what is inflexible?
I know you do, however there are still some areas that are hampered by the chosen design...for example, in a CRTP based design the implementation/backend wrapper would only need to specify the supported pixel formats (like with an MPL typelist) and the switch, based on the input format, could then be done in a single place in the CRTP base class which would then invoke a template member function of the derived class specifying to it, through template parameters, the exact source and target types... But yes, the word 'inflexible' is an incorrect term for the issue I tried to point out here...maybe 'cumbersome' or 'duplication prone'...
Back to negativity. Yes there was a bug back in September. I fixed it, not much more I can do about that.
Christian, please do not take my comments personally :) IMO, in a review process, 'dry'/objective/rational comments demonstrated with straightforward arguments with as clear as possible (black-or-white / 'good'-or-'bad') conclusions get the job done in the shortest amount of time. Sure, this type of discourse can leave the author of the library being reviewed feeling as if there is little appreciation of the work he or she has done but, as it often is with feelings, this need not be true. Please note that I have remarked several times that you've done a massive piece of work ;-) The only comment that had a personal tone to it was the one in the beginning of my first post in the "[gil] New IO release" thread (the one that had a bit of an 'I told you so' overtone)...but it is important that we now have an overall open and friendly discussion and willingness to work together for a common cause... ;)
I hope you realize that providing a IO extension is more of a can of worms.
Of course, it's a 'truly massive' piece of work ;)
I meant sharing across backends that support the same types of images (i.e. having to use one set of properties with the LibPNG backend and an entirely different on with a LodePNG backend would not make much sense)...
Or it can be done that way too...as long as they are all in one place...
Thanks again for your long review. I think I lost some nerves along the way but I very much appreciate your efforts.
I'm sorry for the lost nerves...somehow I've come to accept this as normal in these discussions with all the 'Parksinon's bicycle shed', 'straw man', 'ignored argument' and related 'syndromes' inherent in human (vanity tarred) communication... -- "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
participants (3)
-
Christian Henning
-
Domagoj Saric
-
Domagoj Saric