Introducing a ScopedVector method to append another ScopedVector?

[Václav Brožek]

Hi Chromium devs and //base OWNERS interested in ScopedVector,

On a couple of tens of places in Chromium, people need to append a whole ScopedVector to the end of another. Most of the weak_clear callsites are of this type (see, e.g., SwapQueue::DrainMessages). The code is like this:

vector_a.insert(vector_a.end(), vector_b.begin(), vector_b.end());

vector_b.weak_clear();

I'd like to introduce a method to do this, and replace all those callsites with calling that method. It could be something like

void ScopedVector::Append(RValue rhs) {

  insert(end(), rhs.object->begin(), rhs.object->end());

  rhs.object->weak_clear();

}

and would turn the above example into

vector_a.Append(vector_b.Pass());

My reason is not so much sparing the one line per callsite (mostly two, actually, due to line breaks), they are rather increasing the readability of the code, and making it harder to delete weak_clear by accident, or push it off too far below by inserting stuff between the two lines.

Before I make the CL touching all the callsites, I'd like to hear your comments and objections.

Thanks!

Vacalv

[Dana Jansens]

How about porting the insert_and_take() from ScopedPtrVector over?

https://code.google.com/p/chromium/codesearch#chromium/src/cc/base/scoped_ptr_vector.h&l=132

 

Thanks!

Vacalv

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Václav Brožek]

Dana,

Do I understand correctly, that by porting you mean mainly to add the |postition| argument to insert stuff anywhere in the vector?

If yes, then I'm not sure -- the only callsite of inser_and_take I see in the CodeSearch, which supplies a different position than end() is in a unittest. And the insert & weak_erase pattern instances for ScopedVector I saw also always inserts at the end. Adding |position| would cluttering most of the code with "vector.end()," for no big benefit.

What do you think?

[Dana Jansens]

On Thu, Feb 12, 2015 at 1:12 AM, Václav Brožek <va...@chromium.org> wrote:

Dana,

Do I understand correctly, that by porting you mean mainly to add the |postition| argument to insert stuff anywhere in the vector?

Yes. The API for ScopedVector is meant to look like std::vector. There is no function to Append() a vector onto a vector. There is a function to insert() a vector into a vector, using iterators in the other vector. In ScopedPtrVector we just replaced the iterators with a pointer to the other ScopedPtrVector. Bonus points if we can safely use iterators in the other ScopedVector instead, I guess.

 

If yes, then I'm not sure -- the only callsite of inser_and_take I see in the CodeSearch, which supplies a different position than end() is in a unittest. And the insert & weak_erase pattern instances for ScopedVector I saw also always inserts at the end. Adding |position| would cluttering most of the code with "vector.end()," for no big benefit.

What do you think?

I think making the API look more like vector is better.

 

[Václav Brožek]

Thanks, Dana.

I respect the intention to make the ScopedVector API similar to std::vector.

However, introducing the insert_and_take(position, src_start_iter, src_end_iter) has none of the benefits I hoped to get: the old callsites

vector_a.insert(vector_a.end(), vector_b.begin(), vector_b.end());

vector_b.weak_clear();

would turn into very similar and slightly longer

vector_a.insert_and_take(vector_a.end(), vector_b.begin(), vector_b.end());

vector_b.weak_clear();

Sure, the weak_clear() in the second case is optional, but leaving it out is a potential performance hit (iterating and deleting each (null) pointer instead of just flushing the vector).

So it seems like the current state is actually the best.

Thanks!

Vaclav

[Dana Jansens]

On Feb 13, 2015 2:18 AM, "Václav Brožek" <va...@chromium.org> wrote:
>
> Thanks, Dana.
>
> I respect the intention to make the ScopedVector API similar to std::vector.
>
> However, introducing the insert_and_take(position, src_start_iter, src_end_iter) has none of the benefits I hoped to get: the old callsites
>
> vector_a.insert(vector_a.end(), vector_b.begin(), vector_b.end());
> vector_b.weak_clear();
>
> would turn into very similar and slightly longer
>
> vector_a.insert_and_take(vector_a.end(), vector_b.begin(), vector_b.end());
> vector_b.weak_clear();

It would just be clear() now.

> Sure, the weak_clear() in the second case is optional, but leaving it out is a potential performance hit (iterating and deleting each (null) pointer instead of just flushing the vector).

What do you mean by flushing the vector?

[Viet-Trung Luu]

To top-post some a side comment to a slightly-old thread:

It seems to me that any changes to ScopedVector<...> should be made with the view that we'll want to convert them to std::vector<std::unique_ptr<...>> (or std::vector<scoped_ptr<...>>) "real soon now".

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Václav Brožek]

(Sorry for a delayed answer, was on holiday.)

On 13 February 2015 at 17:48, Dana Jansens <dan...@chromium.org> wrote:

On Feb 13, 2015 2:18 AM, "Václav Brožek" <va...@chromium.org> wrote:
>
> Thanks, Dana.
>
> I respect the intention to make the ScopedVector API similar to std::vector.
>
> However, introducing the insert_and_take(position, src_start_iter, src_end_iter) has none of the benefits I hoped to get: the old callsites
>
> vector_a.insert(vector_a.end(), vector_b.begin(), vector_b.end());
> vector_b.weak_clear();
>
> would turn into very similar and slightly longer
>
> vector_a.insert_and_take(vector_a.end(), vector_b.begin(), vector_b.end());
> vector_b.weak_clear();

It would just be clear() now.

> Sure, the weak_clear() in the second case is optional, but leaving it out is a potential performance hit (iterating and deleting each (null) pointer instead of just flushing the vector).

What do you mean by flushing the vector?

By "flushing" I meant ScopedVector::weak_clear().

ScopedVector::clear() deletes every pointer held in the vector, and then frees the memory used to hold the pointers themselves. weak_clear() does just the latter. The former takes time linear in the size of the vector, the latter is independent of the vector's size.

When replacing insert() with insert_and_take(), the moved pointers get zeroed in the source vector. Thus deleting them does not do anything, and a ScopedVector only containing NULLs can be weak_clear()-ed instead of clear()-ing. After

  vector_a.insert_and_take(vector_a.end(), vector_b.begin(), vector_b.end());

if I don't care about vector_b, I can just leave it as it is (its destructor will call clear() eventually), or I can clear() it to make it empty, or I can weak_clear() it with the same effect.

Using weak_clear() potentially increases performance, because weak_clear() runs in time independent of the number of elements, and even if clear() is eventually called by the destructor anyway, that call is on a 0-size vector already.

Please let me know if I'm still vague.

(As already said, I don't propose the change of ScopedVector anymore, though.)

Cheers,

Vaclav

[Daniel Cheng]

weak_clear() seems like an inherently dangerous method that we wouldn't want to expose.

Given that std::vector<std::unique_ptr<T>> doesn't support anything analogous, I don't think it's worth it.

Daniel

[Václav Brožek]

On 24 February 2015 at 16:57, Daniel Cheng <dch...@chromium.org> wrote:

weak_clear() seems like an inherently dangerous method that we wouldn't want to expose.

But we do expose ScopedVector::weak_clear(), and it is currently necessary for moving ownership out of ScopedVector.

Given that std::vector<std::unique_ptr<T>> doesn't support anything analogous, I don't think it's worth it.

Please clarify -- what do you mean by the second-last "it"?

[Brett Wilson]

As Trung said, ScopedVector is really a stopgap until we can use a vector of unique ptrs. We wouldn't want to do anything to bring us further from that, as this proposal does.

Brett

[Avi Drissman]

I thought we had linked_ptr so that we could use it in containers. I'd rather phase out ScopedVector today in favor of vector<linked_ptr<x>> and later migrate away from linked_ptr. But that's just me.

[Václav Brožek]

Yes, as I pointed out before, I've taken back my proposal.

I'm merely answering follow-up questions, but do not intend to modify ScopedVector.

[Peter Kasting]

On Tue, Feb 24, 2015 at 8:42 AM, Avi Drissman <a...@chromium.org> wrote:

I thought we had linked_ptr so that we could use it in containers. I'd rather phase out ScopedVector today in favor of vector<linked_ptr<x>> and later migrate away from linked_ptr. But that's just me.

Usage of linked_ptr is difficult to get rid of.  Please don't introduce more of it, especially where it's not "really needed" (i.e. it's just standing in for unique_ptr, we don't really need the refcounting aspect), as it's hard to figure out later how it's being used and what it can be safely changed to.  For example, the extensions system uses linked_ptr pervasively in certain areas and I spent a long time trying to figure out if we could migrate away from it there before giving up in confusion.

It's much better to go the opposite way: convert vector<linked_ptr> to ScopedVector, since we can definitely replace ScopedVector with vector<unique_ptr> someday as a mechanical change.  https://code.google.com/p/chromium/issues/detail?id=137767 covers this (but is now unowned since there are only so many cleanup tasks I can handle).

PK