Closed Bug 976373 Opened 6 years ago Closed 6 years ago

Collect mutation records to a list, not to an array

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(2 files)

Attached patch patchSplinter Review
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.
Attachment #8381032 - Flags: review?(continuation)
We've got a nice LinkedList abstraction in mfbt, btw.
Yeah, I was thinking of suggesting that, but does it support having the next pointer be an nsRefPtr?
That LinkedList is not nice in any way, especially not for strong references.
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);|?
Attachment #8381032 - Flags: review?(continuation) → review+
Attached patch v2Splinter Review
https://hg.mozilla.org/mozilla-central/rev/7ea5a64f9279
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.