Closed Bug 1377993 Opened 3 years ago Closed 3 years ago

Make node slots less memory hungry in common cases

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(3 files)

.
Priority: -- → P1
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
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
Looks like stylo is doing something silly with slots :/
like not accessing slot properties using getters
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
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(cam)
(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.
Attached patch slots-rust.diffSplinter Review
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 :)
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(cam)
XULElement has its own mBindingParent. I just simplified the setup in extended slots since memory usage there is less a concern with the patch.
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
Attachment #8884637 - Flags: review?(cam)
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)
Attachment #8884446 - Flags: review?(peterv)
Comment on attachment 8884637 [details] [diff] [review]
slots-rust.diff

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

r=me with an appropriate commit message.
Attachment #8884637 - Flags: review?(cam) → review+
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.
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 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.
Attachment #8884446 - Flags: review?(peterv) → review+
yeah, I don't know why we have ClearHasID there.
ok, apparently landing patches which require changes to stylo is totally broken. Can't work on this bug.
Assignee: bugs → nobody
(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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5fb7a6989ca4
Make node slots less memory hungry in common cases. r=peterv
https://hg.mozilla.org/mozilla-central/rev/5fb7a6989ca4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Are there any numbers for memory savings (per node type and/or overall) this change may have caused?
Assignee: nobody → bugs
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.