[Patch] Efficient ByteVector::replace() implementation

Hello,

I noticed that TagLib will perform exceptionally badly on some
occasions. One extreme example that I've been looking at was an MP3 file
with an album image in BMP format - 7 MB. Extracting that image via
TagLib took definitely more than 10 minutes, I didn't have the patience
to wait for it to be done. All the time was being spent in
ByteVector::replace() method which performs poorly. Here is a patch for
that method:
http://bugzilla.songbirdnest.com/attachment.cgi?id=12077&action=diff 
(http://bugzilla.songbirdnest.com/attachment.cgi?id=12077&action=diff&format=raw 
for the raw patch). The new algorithm never resizes the vector more than
once, it also doesn't perform any unnecessary move operations. The file
in question is processed in less than a second.

regards
Wladimir



_______________________________________________
taglib-devel mailing list
taglib-devel@...
https://mail.kde.org/mailman/listinfo/taglib-devel

On Thu, Sep 17, 2009 at 8:34 AM, Wladimir Palant
<palant@...> wrote:

> I noticed that TagLib will perform exceptionally badly on some occasions.
> One extreme example that I've been looking at was an MP3 file with an album
> image in BMP format - 7 MB. Extracting that image via TagLib took definitely
> more than 10 minutes, I didn't have the patience to wait for it to be done.
> All the time was being spent in ByteVector::replace() method which performs
> poorly. Here is a patch for that method:
> http://bugzilla.songbirdnest.com/attachment.cgi?id=12077&action=diff
> (http://bugzilla.songbirdnest.com/attachment.cgi?id=12077&action=diff&format=raw
> for the raw patch). The new algorithm never resizes the vector more than
> once, it also doesn't perform any unnecessary move operations. The file in
> question is processed in less than a second.

Any chance I could convince you to write unit tests for the new code
paths in ByteVector::replace()?

--
Lukas Lalinsky
lalinsky@...
_______________________________________________
taglib-devel mailing list
taglib-devel@...
https://mail.kde.org/mailman/listinfo/taglib-devel

On 17.09.2009 09:21, Lukáš Lalinský wrote:
> Any chance I could convince you to write unit tests for the new code
> paths in ByteVector::replace()?

The chances are slim. I didn't manage to compile TagLib via the
"official" build system and Songbird's makefile doesn't build unit
tests. If you want to accept some untested unit tests - sure.

regards
Wladimir



_______________________________________________
taglib-devel mailing list
taglib-devel@...
https://mail.kde.org/mailman/listinfo/taglib-devel

2009/9/17 Wladimir Palant <palant@...>:
> On 17.09.2009 09:21, Lukáš Lalinský wrote:
>>
>> Any chance I could convince you to write unit tests for the new code
>> paths in ByteVector::replace()?
>
> The chances are slim. I didn't manage to compile TagLib via the "official"
> build system and Songbird's makefile doesn't build unit tests. If you want
> to accept some untested unit tests - sure.

No, untested tests are not useful. :) The thing is, while I maintain
TagLib, I'll not commit any non-trivial patches without tests to make
sure we are not breaking things in the future. This is not really
about slowing anybody down, it's about code quality. In this case,
I'll have to write the tests, so it will take a little longer.

I've filled https://bugs.kde.org/show_bug.cgi?id=207664 to not forget
about it. Thank you for writing and sending the patch!

--
Lukas Lalinsky
lalinsky@...
_______________________________________________
taglib-devel mailing list
taglib-devel@...
https://mail.kde.org/mailman/listinfo/taglib-devel