Skip to content

Commit

Permalink
Optimize blink::Vector special constructors and assign()
Browse files Browse the repository at this point in the history
In crrev.com/c/6181046, the following blink::Vector constructors were
combined to call blink::Vector::assign:
1. Vector(vector with different inline capacity)
2. Vector(same vector type, projection)
3. Vector(vector with different inline capacity, projection)
4. Vector(range)

blink::Vector::assign calls reserve (in GCForbiddenScope) and
base::ranges::transform(back_inserter), which has lower performance
than the original UinitializedCopy().

Optimizations:
- For #1, add back the original code which can use memcpy when
  possible.
- For #2, #3, #4, call a new function
  TypeOperations::UninitializedTransform() (which is basically
  equivalent to TypeOperations::UninitializedCopy() before
  crrev.com/c/6181046, but accepts generic input iterator),
  instead of assign().
- For assign(), remove GCForbiddenScope [1], and use
  UninitializedTransform() instead of transform(back_inserter).

The differences between this CL and the version before
crrev.com/c/6181046 are:
- Vector(range) now uses optimized code path;
- Vector::assign(range) is optimized.

[1] About GCForbiddenScope: It was added to fix crbug.com/40448463.
It prevents GC during reserve/resize to prevent the change of
the input collection (if it's a hash table with WeakMember keys)
during GC. Before that fix, the result vector might contain extra
empty elements. With the new code, the size of the result vector is
based on the final size of the range, so the original problem no
longer exists. The capacity may be larger than necessary in rare
cases, which is not a big deal.
https://pinpoint-dot-chromeperf.appspot.com/job/16bf9884a10000 shows that removing GCForbiddenScope can improve performance of a benchmark
by 5% (though the improvement will not apply with the final CL
because the assign() code path is no longer used for that benchmark,
which can get a greater improvement).

Bug: 390461329
Change-Id: I152218d56ab93c44ca89f3397d96b0b7780edd6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6185833
Reviewed-by: Kentaro Hara <[email protected]>
Reviewed-by: Michael Lippautz <[email protected]>
Commit-Queue: Xianzhu Wang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1410367}
  • Loading branch information
wangxianzhu authored and Chromium LUCI CQ committed Jan 23, 2025
1 parent 3965e78 commit 2c082a6
Showing 1 changed file with 67 additions and 13 deletions.
80 changes: 67 additions & 13 deletions third_party/blink/renderer/platform/wtf/vector.h
Original file line number Diff line number Diff line change
Expand Up @@ -363,15 +363,28 @@ struct VectorTypeOperations {
}
if constexpr (std::is_same_v<T, U> && VectorTraits<T>::kCanCopyWithMemcpy) {
Copy(src, src_end, dst, origin);
} else if (origin == VectorOperationOrigin::kConstruction) {
} else {
UninitializedTransform(src, src_end, dst, origin, std::identity());
}
}

template <typename InputIterator, typename Proj>
static void UninitializedTransform(InputIterator src,
InputIterator src_end,
T* dst,
VectorOperationOrigin origin,
Proj proj) {
if (origin == VectorOperationOrigin::kConstruction) {
while (src != src_end) {
ConstructTraits::Construct(dst, *src);
ConstructTraits::Construct(
dst, std::invoke(proj, std::forward<decltype(*src)>(*src)));
++dst;
++src;
}
} else {
while (src != src_end) {
ConstructTraits::ConstructAndNotifyElement(dst, *src);
ConstructTraits::ConstructAndNotifyElement(
dst, std::invoke(proj, std::forward<decltype(*src)>(*src)));
++dst;
++src;
}
Expand Down Expand Up @@ -1254,6 +1267,8 @@ class Vector : private VectorBuffer<T, INLINE_CAPACITY, Allocator> {

// Copying.
Vector(const Vector&);
template <wtf_size_t otherCapacity>
explicit Vector(const Vector<T, otherCapacity, Allocator>&);

Vector& operator=(const Vector&);
template <wtf_size_t otherCapacity>
Expand All @@ -1263,9 +1278,8 @@ class Vector : private VectorBuffer<T, INLINE_CAPACITY, Allocator> {
// optional projection.
template <typename Range, typename Proj = std::identity>
requires VectorCanAssignFromRange<T, InlineCapacity, Allocator, Range, Proj>
explicit Vector(Range&& range, Proj proj = {}) : Vector() {
assign(std::forward<Range>(range), std::move(proj));
}
explicit Vector(Range&&, Proj = {});

// Replaces the vector with elements copied from an input and sized range.
template <typename Range, typename Proj = std::identity>
requires VectorCanAssignFromRange<T, InlineCapacity, Allocator, Range, Proj>
Expand Down Expand Up @@ -1726,6 +1740,34 @@ Vector<T, InlineCapacity, Allocator>::Vector(const Vector& other)
VectorOperationOrigin::kConstruction);
}

template <typename T, wtf_size_t InlineCapacity, typename Allocator>
template <wtf_size_t otherCapacity>
Vector<T, InlineCapacity, Allocator>::Vector(
const Vector<T, otherCapacity, Allocator>& other)
: Base(other.capacity()) {
ANNOTATE_NEW_BUFFER(data(), capacity(), other.size());
size_ = other.size();
TypeOperations::UninitializedCopy(other.data(), other.DataEnd(), data(),
VectorOperationOrigin::kConstruction);
}

template <typename T, wtf_size_t InlineCapacity, typename Allocator>
template <typename Range, typename Proj>
requires VectorCanAssignFromRange<T, InlineCapacity, Allocator, Range, Proj>
Vector<T, InlineCapacity, Allocator>::Vector(Range&& other, Proj proj)
: Base(std::ranges::size(other)) {
// Note that `size(other)` may become smaller if `other` is a hash table
// with WeakMember keys and `Base(size(other))` above caused GC which
// removed some entries from `other`, see crbug.com/40448463. This won't
// cause problems as long as we won't use the old `size(other)` in the
// following code.
ANNOTATE_NEW_BUFFER(data(), capacity(), std::ranges::size(other));
TypeOperations::UninitializedTransform(
std::ranges::begin(other), std::ranges::end(other), data(),
VectorOperationOrigin::kConstruction, std::move(proj));
size_ = std::ranges::size(other);
}

template <typename T, wtf_size_t InlineCapacity, typename Allocator>
Vector<T, InlineCapacity, Allocator>&
Vector<T, InlineCapacity, Allocator>::operator=(
Expand Down Expand Up @@ -1791,15 +1833,27 @@ Vector<T, InlineCapacity, Allocator>::operator=(
template <typename T, wtf_size_t InlineCapacity, typename Allocator>
template <typename Range, typename Proj>
requires VectorCanAssignFromRange<T, InlineCapacity, Allocator, Range, Proj>
void Vector<T, InlineCapacity, Allocator>::assign(Range&& range, Proj proj) {
{
// Disallow GC across resize allocation, see crbug.com/568173.
GCForbiddenScope scope;
reserve(base::checked_cast<wtf_size_t>(std::ranges::size(range)));
void Vector<T, InlineCapacity, Allocator>::assign(Range&& other, Proj proj) {
if (std::ranges::size(other) > capacity()) {
clear();
reserve(std::ranges::size(other));
// Note that `size(other)` may become smaller if `other` is a hash table
// with `WeakMember` keys and `reserve` caused GC which removed some
// entries from `other`, see crbug.com/40448463. This won't cause problems
// as long as we won't use the old `size(other)` in the following code.
} else {
if (std::ranges::size(other) < size()) {
Shrink(std::ranges::size(other));
}
TypeOperations::Destruct(data(), DataEnd());
}

base::ranges::transform(std::forward<Range>(range), std::back_inserter(*this),
std::move(proj));
MARKING_AWARE_ANNOTATE_CHANGE_SIZE(Allocator, data(), capacity(), size_,
std::ranges::size(other));
TypeOperations::UninitializedTransform(
std::ranges::begin(other), std::ranges::end(other), data(),
VectorOperationOrigin::kRegularModification, std::move(proj));
size_ = std::ranges::size(other);
}

template <typename T, wtf_size_t InlineCapacity, typename Allocator>
Expand Down

0 comments on commit 2c082a6

Please sign in to comment.