Re: [Libreoffice] [PATCH] ScColumn - convert array to vector

On Fri, 2012-01-13 at 20:36 +0200, Noel Grandin wrote:
> Hi
>
> Oh no!, I was testing some stuff and accidentally commented out the
> critical line in ScColumn::Append().
>
> This line
> +//    aItems.resize(nSize);
> should be
> +    aItems.resize(nSize);

I guess you meant ScColumn::Resize()?  Yup, I'll fix that.

> We could call reserve() on the vector to allocate extra capacity, but
> I don't think there is any way to get exactly the same behaviour as
> before in the ScColumn::Append() method.

Yeah that's fine.  The double allocation behavior doesn't have to be
identical before and after the change, as long as the import performance
is comparable to what it was prior to the change.

Indeed, I just ran a quick test, and the test ods file I used which
opens in 6.5 seconds on my machine opens in 6.6 seconds after your
change.  I believe std::vector's push_back already does double
allocation of sort, in which case we are safe, performance-wise.

As a comparison, in the old code, when ScColumn::bDoubleAlloc is set to
false, the same file opens so slow that you need to kill the process.
That's how much difference doing double allocation makes.

Now, the only issue we need to be concerned about is that, if we decide
to always perform double allocation whether it's import or run-time
(which your change will do), we may end up allocating larger array than
necessary even when we are accepting user's input.  But I'm willing to
accept that for the time being, and maybe later re-work it if that
proves to cause memory footprint issue later on.

There are two minor changes I'd like to make.  One is to rename the
structure from aItems to maItems, to make it explicit that it's a data
member.  A bunch of existing code doesn't follow this convention, but
I'd like us to start using this convention in our new code.  Two is to
use back() method to access the last item of the array.  So, instead of

  aItems[aItems.size()-1]

to access the last array item, you can simply do

  aItems.back()

which is much simpler.  I'll make these changes for you.

Let me do a thorough review of your patch one more time, and I'll push
to master.

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc