976373 - Collect mutation records to a list, not to an array

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Currently we collect mutation records to a list, but that is malloc/free heavy in 
case we have lots of records. So we could just collect them to a list and
create the result array on demand.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We've got a nice LinkedList abstraction in mfbt, btw.

Comment 2

[Andrew McCreight [:mccr8]]

Yeah, I was thinking of suggesting that, but does it support having the next pointer be an nsRefPtr?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

That LinkedList is not nice in any way, especially not for strong references.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8381032 [details] [diff] [review]
patch

Review of attachment 8381032 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDOMMutationObserver.cpp
@@ +601,5 @@
>    mozilla::dom::Sequence<mozilla::dom::OwningNonNull<nsDOMMutationRecord> >
>      mutations;
> +  if (mutations.SetCapacity(mPendingMutationCount)) {
> +    // We can't use TakeRecords easily here, because it deals with
> +    // different type of array, and we want to optimize out any extra copying.

nit: "with different" -> "with a different"

@@ +602,5 @@
>      mutations;
> +  if (mutations.SetCapacity(mPendingMutationCount)) {
> +    // We can't use TakeRecords easily here, because it deals with
> +    // different type of array, and we want to optimize out any extra copying.
> +    nsRefPtr<nsDOMMutationRecord> current;

If you wanted to avoid code duplication here, you could turn this loop into a template, and then instantiate it in the two places you need to do a copy loop.

::: content/base/src/nsDOMMutationObserver.h
@@ +392,5 @@
> +    if (!mLastPendingMutation) {
> +      MOZ_ASSERT(!mFirstPendingMutation);
> +      mFirstPendingMutation = aRecord;
> +      mLastPendingMutation = mFirstPendingMutation;
> +    } else {

Maybe |MOZ_ASSERT(mFirstPendingMutation);|?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea5a64f9279

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ea5a64f9279