1377993 - Make node slots less memory hungry in common cases

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

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

.

Comment 1

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

Attachment: extended_slots.diff

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcc0a167e1251583a74bebdabb16156b191d6a3d
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=fcc0a167e1251583a74bebdabb16156b191d6a3d

Comment 2

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

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=23d79f3b06ed35182a5ef12193b7440912d0e568
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=23d79f3b06ed35182a5ef12193b7440912d0e568

Comment 3

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

Looks like stylo is doing something silly with slots :/

Comment 4

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

like not accessing slot properties using getters

Comment 5

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

heycam, emilio, could you perhaps hint to me what to change. 
See https://treeherder.mozilla.org/logviewer.html#?job_id=112758117&repo=try&lineNumber=7077

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #4)
> like not accessing slot properties using getters

We'd need to go through the FFI boundary for that, and for a few of those it's unacceptable, I think, so we need to add the same getters on the rust side.

I'll cherry-pick your patch and post a patch for the rust bits here.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: slots-rust.diff

This should work.

I guess now that we always have a mBindingParent pointer in the DOM slots, we could use that for XUL elements too instead of a virtual call, if so, you'd need to remove the `if self.is_xul_element()` branch in `get_xbl_binding_parent`, and rename and remove the assertion inside `get_non_xul_xbl_binding_parent_raw_content`.

Happy to do that if you want too :)

Comment 8

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

XULElement has its own mBindingParent. I just simplified the setup in extended slots since memory usage there is less a concern with the patch.

Comment 9

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

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebbe3b55a8a2ffd9928e6b455a928a47b14893d3
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=ebbe3b55a8a2ffd9928e6b455a928a47b14893d3

Comment 10

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

Comment on attachment 8884446 [details] [diff] [review]
extended_slots.diff

I merged XUL slots, removed that one union in slots and then the setup was simpler. Based on local testing kept often used properties in nsDOMSlots and moved rarely used to nsExtendedDOMSlots (and used ns-prefix for consistency with nsDOMSlots)

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8884637 [details] [diff] [review]
slots-rust.diff

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

r=me with an appropriate commit message.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8884446 [details] [diff] [review]
extended_slots.diff

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

::: dom/base/FragmentOrElement.h
@@ +271,5 @@
>      RefPtr<mozilla::DeclarationBlock> mSMILOverrideStyleDeclaration;
>  
>      /**
> +    * The nearest enclosing content node with a binding that created us.
> +    * @see FragmentOrElement::GetBindingParent

Can you please comment here that this will be null for XUL elements, which use nsXULElement::mBindingParent instead.

Comment 13

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

I may consider to make XUL to use slots too for binding parent.
sXULElementCount 2304, sBindingParentCount 597.
Perhaps in that case I'd move binding parent from extended slots to normal slots.
But that isn't about this bug.

(if we could make nsXULElement not inherit nsIDOMXULElement and not have mBindingParent, its size would be nicely 128 on 64bit, I think)

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 8884446 [details] [diff] [review]
extended_slots.diff

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

I think we need to think of a way to start tracking the usage of slots, it might change over time depending on what features become popular or fade away. We do end up reacting to profiles during performance pushes, but it'd be nice to be prompted instead of waiting for someone to profile. I don't have any bright ideas though, telemetry might be a bit heavyweight.

::: dom/base/FragmentOrElement.cpp
@@ +691,5 @@
> +  }
> +
> +  NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mExtendedSlots->mSMILOverrideStyle");
> +  cb.NoteXPCOMChild(mExtendedSlots->mSMILOverrideStyle.get());
> +  

Trailing whitespace.

@@ +757,5 @@
> +  mExtendedSlots->mRegisteredIntersectionObservers.Clear();
> +  nsCOMPtr<nsIFrameLoader> frameLoader =
> +    do_QueryInterface(mExtendedSlots->mFrameLoaderOrOpener);
> +  if (frameLoader) {
> +    static_cast<nsFrameLoader*>(frameLoader.get())->Destroy();

Shouldn't this set mFrameLoaderOrOpener to null? Or you could maybe even set mExtendedSlots to null?

::: dom/xul/nsXULElement.cpp
@@ +305,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsXULElement,
>                                                  nsStyledElement)
>      // Why aren't we unlinking the prototype?
>      tmp->ClearHasID();

I wonder why we need this, it seems weird that unlinking needs to clear a boolean flag. It'd be nice to be able to remove nsXULElement's CC code given how little it does.

Comment 15

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

yeah, I don't know why we have ClearHasID there.

Comment 16

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

Attachment: extended_slots_2.diff

Comment 17

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

ok, apparently landing patches which require changes to stylo is totally broken. Can't work on this bug.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #17)
> ok, apparently landing patches which require changes to stylo is totally
> broken. Can't work on this bug.

Hm, can you elaborate? The normal way to land the changes is to land the servo side, wait for it to sync over, and then land the gecko side.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fb7a6989ca4
Make node slots less memory hungry in common cases. r=peterv

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Servo bits landed right before in https://hg.mozilla.org/integration/autoland/rev/b37994ac99a3

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fb7a6989ca4

Comment 22

[Florian Bender]

Are there any numbers for memory savings (per node type and/or overall) this change may have caused?