Replace the nsRange HTMLMediaElement::mSourcePointer with something faster

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mats, Assigned: mats)

Tracking

({perf})

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
It looks like every HTMLMediaElement (<audio> and <video> I presume) creates
a nsRange in case there are <source> children:
https://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/dom/﷒0﷓
it seems this nsRange exist for the lifetime of the HTMLMediaElement.

nsRange is a mutation observer of its root node though, which slows down
DOM changes, so I'd like to get rid of this nsRange if possible.

I think the reason for using a nsRange here is because the spec requires
the "pointer" to be updated in response to DOM changes:
https://dev.w3.org/html5/spec-preview/media-elements.html#concept-media-load-algorithm
"As nodes are inserted and removed into the media element,
/pointer/ must be updated as follows [...]"

Using a nsRange for this purpose seems like overkill, we can just
let the media element observe itself to detect such changes.

Here's a WIP patch for doing that:
https://hg.mozilla.org/try/rev/889f13632c88

Looks green so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43f4158d2e9d249986863289f8e87b2d9e92cf4f

Chris, do you think something simpler like this could work?
Flags: needinfo?(cpearce)
(In reply to Mats Palmgren (:mats) from comment #0)
> It looks like every HTMLMediaElement (<audio> and <video> I presume) creates
> a nsRange in case there are <source> children:
> https://searchfox.org/mozilla-central/rev/
> 7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/dom/html/HTMLMediaElement.cpp#6362
> it seems this nsRange exist for the lifetime of the HTMLMediaElement.
> 
> nsRange is a mutation observer of its root node though, which slows down
> DOM changes, so I'd like to get rid of this nsRange if possible.

You make a very convincing argument. :)

 
> I think the reason for using a nsRange here is because the spec requires
> the "pointer" to be updated in response to DOM changes:
> https://dev.w3.org/html5/spec-preview/media-elements.html#concept-media-load-
> algorithm
> "As nodes are inserted and removed into the media element,
> /pointer/ must be updated as follows [...]"

Yes, that matches my recollection.


> Using a nsRange for this purpose seems like overkill, we can just
> let the media element observe itself to detect such changes.
> 
> Here's a WIP patch for doing that:
> https://hg.mozilla.org/try/rev/889f13632c88
> 
> Looks green so far:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=43f4158d2e9d249986863289f8e87b2d9e92cf4f
> 
> Chris, do you think something simpler like this could work?

I think https://searchfox.org/mozilla-central/source/dom/media/test/test_media_selection.html should exercise the code changed here. If what you have done here didn't work, that test should have failed.

So provided we still observe mutations to an HTMLMediaElement's <source> children while the async load algorithm is running, as appears to be happening, then we should be good here. Ship it!
Flags: needinfo?(cpearce)

Updated

a year ago
Component: Audio/Video → Audio/Video: Playback
(Assignee)

Comment 2

a year ago
Created attachment 8880004 [details] [diff] [review]
Use simpler mutation observer than nsRange for media elements

Yeah, this should notify all changes in the media element subtree,
and I think we can ignore all notifications except insert/remove
(as the spec points out), and all of those that isn't a direct child
(aContainer != this).

The test_media_selection.html test pass locally and in the Try run.

BTW, this also removes the only use of SetEnableGravitationOnElementRemoval
so I'll remove that nsRange method in a follow-up bug.
Attachment #8880004 - Flags: review?(cpearce)
(Assignee)

Updated

a year ago
Blocks: 1375097
Comment on attachment 8880004 [details] [diff] [review]
Use simpler mutation observer than nsRange for media elements

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

Great, thanks!
Attachment #8880004 - Flags: review?(cpearce) → review+

Updated

a year ago
Whiteboard: [qf] → [qf:p1]

Comment 4

a year ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/463d13ce3e83
Use simpler mutation observer than nsRange for media elements.  r=cpearce

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/463d13ce3e83
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Can we make mSourcePointer a node pointer instead of an integer offset?
I'm working on a set of patches for bug 1330970 and providing an index on mutation notifications is going to
be expensive. Also, GetChildAt will be slower. Keeping a node reference around and using GetNextSibling()
will help avoid the perf hit.
Flags: needinfo?(cpearce)
Ah, linked the wrong bug. It's bug 651120.
(In reply to Cătălin Badea (:catalinb) from comment #6)
> Can we make mSourcePointer a node pointer instead of an integer offset?
> I'm working on a set of patches for bug 1330970 and providing an index on
> mutation notifications is going to
> be expensive.


The key thing we have to maintain here is that we need to ensure that the asynchronous HTMLMediaElement load algorithm works if HTMLSourceElement children are removed or added while it the load algorithm is running. So provided we could recover from the node being pointed to by the node pointer being removed, and provided we'd traverse nodes appended as children to the media element while the algorithm is running, I don't have a problem with this change being made.


> Also, GetChildAt will be slower. Keeping a node reference
> around and using GetNextSibling()
> will help avoid the perf hit.

Will your change affect the performance of any web facing APIs?
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #8)
> (In reply to Cătălin Badea (:catalinb) from comment #6)
> > Can we make mSourcePointer a node pointer instead of an integer offset?
> > I'm working on a set of patches for bug 1330970 and providing an index on
> > mutation notifications is going to
> > be expensive.
> 
> 
> The key thing we have to maintain here is that we need to ensure that the
> asynchronous HTMLMediaElement load algorithm works if HTMLSourceElement
> children are removed or added while it the load algorithm is running. So
> provided we could recover from the node being pointed to by the node pointer
> being removed, and provided we'd traverse nodes appended as children to the
> media element while the algorithm is running, I don't have a problem with
> this change being made.
> 
> 
> > Also, GetChildAt will be slower. Keeping a node reference
> > around and using GetNextSibling()
> > will help avoid the perf hit.
> 
> Will your change affect the performance of any web facing APIs?
nsInode::GetChildAt becomes O(n) and IndexOf won't benefit from the same cache as before.
I'm working on fixing these issues by removing their usage as much as possible and using
caching where possible. On the other side we gain on basic tree operations such as InsertBefore and RemoveChild.
See Also: → bug 1380621
You need to log in before you can comment on or make changes to this bug.