1259311 - twitter takes much longer to load new tweets in firefox compared to chrome

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

STR:

1) Log in to twitter.
2) Let the timeline sit for a while until a large number of "new tweets" are shown at the top.
3) Click the "Home" icon in the upper left to load the new tweets.

On chrome loading ~500 tweets takes 2 to 3 seconds on my windows 10 desktop.

On Firefox nightly loading ~500 tweets takes up to 10 seconds on the same machine.  This triggers the slow script warning.

I profiled a non-e10s build of nightly in visual studio:

43% of the time is spent in PresShell::FlushPendingNotifications.  About half of this time is directly triggered as a sync reflow from accessing .offsetWidth.

Its unclear to me why chrome is so much faster at this point.

I plan to investigate this over time since its something that annoys me on a daily basis.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

JS profiling in devtools suggests the bulk of the reflows is being triggered by a function:

    this.addEllipses = function () {
      var a = this.select('unEllipsifiedTextSelector').filter(':visible');
      a.removeClass(this.attr.unEllipsifiedTextClass),
      a.each(this.addEllipsis.bind(this))
    },
    this.addEllipsis = function (a, b) {
      var c = $(b),
      d = this.createRange(c);
      if (!d.hasOverflow()) return;
      c.data('full-text', c.text()),
      d.shortenToVisibleContent(),
      d.ensureLastLineAvailableWidth(this.maxEllipsisWidth(c)),
      d.shortenToNearestWordBreak({
        maxCharsToRemove: this.attr.maxCharsRemoveEnsureEndOnWordBreak
      });
      var e = d.toDocumentFragment();
      e.appendChild(document.createTextNode('…')),
      c.html(e),
      this.trigger(c, 'uiEllipsisAdded')
    },
    this.maxEllipsisWidth = function (a) {
      var b = parseInt(a.css('font-size'));
      return b * 2
    },
    this.createRange = function (a) {
      return new MultilineTextRange(a)
    }

I can't find a way to copy the stack out of the devtools profiler, so here is a screenshot of the stack:

  https://www.dropbox.com/s/54sr6vts3djvndv/Screenshot%202016-03-23%2022.39.32.png?dl=0

The two functions immediately above the style recalc are from jquery 1.11.2:

  n.expr.filters.hidden = function (a) {
    return a.offsetWidth <= 0 && a.offsetHeight <= 0 || !l.reliableHiddenOffsets() && (a.style && a.style.display || n.css(a, 'display')) === 'none'
  };
  n.expr.filters.visible = function (a) {
    return !n.expr.filters.hidden(a)
  };

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

In both Firefox and Chrome I see two main sync reflows being triggered.  They are both triggered by the same js as far as I can tell.

The main difference is that chrome recalculates style much faster than we do.  For a minimal number of tweets loaded I see:

Nightly: 190ms and 257ms reflows
Chrome:   67ms and  28ms reflows

I tried disabling async stacks in nightly, but it did not help.

I also see a very large number of tiny restyles in firefox nightly that do not show in the chrome devtools.  These are each about 1ms, but there are quite a lot of them.  Its possible they exist on chrome as well, but are too small for me to see in the tools.

Comment 3

[Cameron McCormack (:heycam)]

Thanks for filing this, Ben.  twitter.com has been slow for a long time and I'm ashamed I never filed a bug. :-)

The first thing that would be interesting to know is what DOM modification caused that eRestyle_Subtree request to be added since the previous style flush, by checking the stack at the time RestyleTracker::AddPendingRestyle(..., eRestyle_Subtree | ..., ...) is called.  It'll probably be RestyleManager::AttributeChanged or some other DOM mutation notification method on RestyleManager.  Then we could look at why eRestyle_Subtree (which means re-run selector matching and recompute styles for the element and all of its descendants) is generated for that modification, and whether that makes sense (based on what rules are in style sheets).  And it would be good to know what the DOM and frame trees look like.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Note, the numbers for firefox nightly in comment 2 were from an opt-debug build.  So they are a bit pessimistic.  Using a vanilla opt night gives:

Nightly: 109ms and 144ms
Chrome:   67ms and  28ms

Nightly still sees the large number of tiny reflows as well.

This particular run I did also triggered an 80ms incremental GC as well.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

This seems to be a perf issue with the style system.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Further improved numbers.  This time I refreshed the page in both chrome and firefox before updating with a single new tweet pending:

Nightly:
  - Handling click event took total 173ms
  - Reflows related took total of 88.15ms
    - Recalculate style took 39.32ms
    - Layout took 58.83ms

Chrome:
  - Handling click event took total 109ms
  - Reflow related took total of 48.2ms
    - Recalculate style took 37.64ms
    - Layout took 10.52ms

So this suggests its actually the Layout that is more significantly different than the restyle.  At a minimal workload anyway.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I did another comparison with a large number of tweets.  244 for nightly and 270 for chrome.

Nightly:
  - Total click event handling 5,712ms
  - Total Restyle 1,901.4ms
  - Total Layout 2,767.6ms

Chrome:
  - Total click event handling 3,061ms
  - Total Restyle 759.8ms
  - Total Layout 1436.4ms

So with a larger workload restyling becomes an issue again.  Layout also remains an issue.

I guess we are probably looking at more nodes than necessary or something as various people have suggested.

Comment 8

[Daniel Holbert [:dholbert]]

[I'll bump this back to Layout then; sorry for preemptively reclassifying]

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I should also mention on the run in comment 7, I did see a number of smaller restyles/layouts in chrome as well.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Based on the chrome devtools, the long list of smaller restyles/layouts is due to the `dedupeAndCollapseNew()` function.  This seems to collapse conversation threads, etc, in the timeline.  This in turns seems to call some `slideAndFadeContent()` function which touches sync reflow attributes.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Cameron, I caught the following stacks in RestyleTracker.  I might have missed some, but these seem to hit a lot.  Basically, it inserts a bunch of data, then sets some class names, then touches sync reflow attributes.

aRestyleHint == 12
>	xul.dll!mozilla::RestyleTracker::AddPendingRestyleToTable(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, const mozilla::RestyleHintData * aRestyleHintData) Line 427	C++
 	xul.dll!mozilla::RestyleTracker::AddPendingRestyle(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, const mozilla::RestyleHintData * aRestyleHintData, mozilla::Maybe<mozilla::dom::Element *> aRestyleRoot) Line 514	C++
 	xul.dll!mozilla::RestyleManager::PostRestyleEvent(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, const mozilla::RestyleHintData * aRestyleHintData) Line 1894	C++
 	xul.dll!mozilla::RestyleSiblingsStartingWith(mozilla::RestyleManager * aRestyleManager, nsIContent * aStartingSibling) Line 1425	C++
 	xul.dll!mozilla::RestyleManager::RestyleForInsertOrChange(mozilla::dom::Element * aContainer, nsIContent * aChild) Line 1481	C++
 	xul.dll!mozilla::RestyleManagerHandle::Ptr::RestyleForInsertOrChange(mozilla::dom::Element * aContainer, nsIContent * aChild) Line 81	C++
 	xul.dll!PresShell::ContentInserted(nsIDocument * aDocument, nsIContent * aContainer, nsIContent * aChild, int aIndexInContainer) Line 4299	C++
 	xul.dll!nsNodeUtils::ContentInserted(nsINode * aContainer, nsIContent * aChild, int aIndexInContainer) Line 201	C++
 	xul.dll!nsINode::doInsertChildAt(nsIContent * aKid, unsigned int aIndex, bool aNotify, nsAttrAndChildArray & aChildArray) Line 1613	C++
 	xul.dll!mozilla::dom::FragmentOrElement::InsertChildAt(nsIContent * aKid, unsigned int aIndex, bool aNotify) Line 1152	C++
 	xul.dll!nsINode::ReplaceOrInsertBefore(bool aReplace, nsINode * aNewChild, nsINode * aRefChild, mozilla::ErrorResult & aError) Line 2261	C++
 	xul.dll!nsINode::InsertBefore(nsINode & aNode, nsINode * aChild, mozilla::ErrorResult & aError) Line 1764	C++
 	xul.dll!mozilla::dom::NodeBinding::insertBefore(JSContext * cx, JS::Handle<JSObject *> obj, nsINode * self, const JSJitMethodCallArgs & args) Line 612	C++



aRestyleHint == 8
>	xul.dll!mozilla::RestyleTracker::AddPendingRestyle(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, const mozilla::RestyleHintData * aRestyleHintData, mozilla::Maybe<mozilla::dom::Element *> aRestyleRoot) Line 514	C++
 	xul.dll!mozilla::RestyleManager::PostRestyleEvent(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, const mozilla::RestyleHintData * aRestyleHintData) Line 1894	C++
 	xul.dll!mozilla::RestyleManager::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 1329	C++
 	xul.dll!mozilla::RestyleManagerHandle::Ptr::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 125	C++
 	xul.dll!PresShell::AttributeChanged(nsIDocument * aDocument, mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 4232	C++
 	xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 146	C++
 	xul.dll!mozilla::dom::Element::SetAttrAndNotify(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAttrValue & aOldValue, nsAttrValue & aParsedValue, unsigned char aModType, bool aFireMutation, bool aNotify, bool aCallAfterSetAttr) Line 2436	C++
 	xul.dll!mozilla::dom::Element::SetAttr(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAString_internal & aValue, bool aNotify) Line 2297	C++
 	xul.dll!nsGenericHTMLElement::SetAttr(int aNameSpaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAString_internal & aValue, bool aNotify) Line 910	C++
 	xul.dll!mozilla::dom::Element::SetAttr(int aNameSpaceID, nsIAtom * aName, const nsAString_internal & aValue, bool aNotify) Line 461	C++
 	xul.dll!mozilla::dom::Element::SetClassName(const nsAString_internal & aClassName) Line 623	C++
 	xul.dll!mozilla::dom::ElementBinding::set_className(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::Element * self, JSJitSetterCallArgs args) Line 474	C++



aRestyleHint == 4
>	xul.dll!mozilla::RestyleTracker::AddPendingRestyle(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aMinChangeHint, const mozilla::RestyleHintData * aRestyleHintData, mozilla::Maybe<mozilla::dom::Element *> aRestyleRoot) Line 514	C++
 	xul.dll!mozilla::RestyleTracker::DoProcessRestyles() Line 192	C++
 	xul.dll!mozilla::RestyleManager::ProcessRestyles(mozilla::RestyleTracker & aRestyleTracker) Line 541	C++
 	xul.dll!mozilla::RestyleManager::ProcessPendingRestyles() Line 1766	C++
 	xul.dll!mozilla::RestyleManagerHandle::Ptr::ProcessPendingRestyles() Line 74	C++
 	xul.dll!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aFlush) Line 4042	C++
 	xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType) Line 3930	C++
 	xul.dll!nsDocument::FlushPendingNotifications(mozFlushType aType) Line 8277	C++
 	xul.dll!mozilla::dom::Element::GetPrimaryFrame(mozFlushType aType) Line 2126	C++
 	xul.dll!mozilla::dom::Element::GetStyledFrame() Line 579	C++
 	xul.dll!nsGenericHTMLElement::GetOffsetRect(mozilla::gfx::IntRectTyped<mozilla::CSSPixel> & aRect) Line 333	C++
 	xul.dll!nsGenericHTMLElement::OffsetWidth() Line 295	C++
 	xul.dll!mozilla::dom::HTMLElementBinding::get_offsetWidth(JSContext * cx, JS::Handle<JSObject *> obj, nsGenericHTMLElement * self, JSJitGetterCallArgs args) Line 1637	C++

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

As far as the DOM tree looks like, AFAICT there is an <ol> containing the twitter timeline.  Each tweet is an <li>.

A conversation of tweets (blue line between tweets) is also an <li>, but it contains its own <ol> with more <li> items for the tweets within that conversation.

Based on the js and what the page is doing when I click the link, I believe its insert <li> elements at the top of the timeline <ol>.  It then goes back over the <li> elements and collapses related tweets into their own nested <ol>.  As part of this step its doing a jquery visibility check which touches offsetWidth and offsetHeight.

I'm not sure how to get the frame tree for the page.  And is there a better way to communicate the DOM structure?  Thanks again for your help!

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I guess I'm seeing a lot "later sibling restyles" in that last stack in comment 11.

I tried enabling restyle logging, but using the environment variable is too verbose.  I can't even get twitter to initially render.  I'll see if I can figure out a convenient way to call StartRestyleLogging().

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Ben Kelly [:bkelly] from comment #12)
> I'm not sure how to get the frame tree for the page.  And is there a better
> way to communicate the DOM structure?  Thanks again for your help!

You can get a dump of the frame tree by calling frame->DumpFrameTree() on any nsIFrame* in the document.  For a dump of the DOM if you can get the nsIDocument* call doc->List((FILE*)stdout, 0).

For restyle logging (with RESTYLE_LOGGING defined in my .mozconfig) I often end up either inserting dump("before\n") / dump("after") calls right before and after the restyle I'm interested in so that I can find the interesting parts of the restyle log, or I add

  partial interface Document {
    void startRestyleLogging();
    void stopRestyleLogging();
  }

and then add some methods on nsIDocument that forward these on to the corresponding methods on nsPresContext.

*Sometimes restyle logging output is so voluminous that it's better to redirect it to a file so that console output and scrolling doesn't slow things down too much.)

It looks like the third AddPendingRestyle stack you show is the expansion of (eRestyle_Subtree | eRestyle_LaterSiblings) into eRestyle_Subtree on the later element siblings that's done in DoProcessRestyles.

From the first stack, I guess we're finding NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS on the node.

From the second stack, I guess we have some rule in the style sheet like

  .a + ...

so when we detect a toggling of the "a" class we generate eRestyle_LaterSiblings.

Both these might be caused by the same types of +/~ selectors in the style sheet.  Can you verify that NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS is being added due to +/~ combinators, or if it's something else (like an :nth-of-type)?  We might have to think about how we can handle restyling more precisely in the presence of these selectors.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: restyle.txt.gz

I finally remembered how to redirect stderr (./mach run 2> foo.txt) and was able to collect the logs.

I truncated the file right before I clicked the "load new tweets" link, but there may still be some unrelated junk at the top of the file.  If you search for "Sibling", though, you can see where its processing the tweets.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I added some printfs and I see NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS being added in two places:

1) For the +/~ selectors:

  https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#2406

2) In nthChildGenericMatches():

  https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#1513

Not sure if its expected or not, but I see the +/~ NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS getting triggered on about:newtab as well.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

I see both ~ and + being uses in the twitter css.  For example:

.stream-item.separated-module+.stream-item,.stream-item.separated-module+.stream-item .tweet:hover,.PromptbirdPrompt-streamItem.separated-module+.stream-item {
	border-top-left-radius:6px;
	border-top-right-radius:6px
}

.conversation-module>li.original-tweet-item ~ li:before,.conversation-module>li.original-tweet-item ~ li:after {
	display:none
}

Comment 18

[Cameron McCormack (:heycam)]

Perhaps the eRestyle_LaterSiblings restyles aren't unnecessary, or at least aren't causing much of the unnecessary restyles.

There are two restyles that each take up about 39% of lines in the log: the ones starting from line 3533 and line 246583.  Both of these are processing an eRestyle_Subtree targetted at the same <div>.  They both result in some change to style data in the Position struct (which generates a reflow change hint), but none of the descendants end up with different style data.  Unfortunately the log doesn't show whether each of the descendants gets a new style context with the same or a different rule node (i.e., whether the selector matching found a different list of matching rules), so we can't tell whether these restyles are wasteful.  So I wonder whether the eRestyle_Subtree that we generate for the <div> is because we know that the style sheet rules could result in descendants matching different rules (in response to whatever DOM change triggered the restyle), or if we can't quite tell that it's safe to return eRestyle_Self (just to restyle that element) for some reason.  So that is what I'd look into next: what specific DOM change caused the restyle to be generated for that <div>, and why it ended up as eRestyle_Subtree.

(This is where using rr would come in handy!  If your log were from a recording, you could replay starting from when line 3533 was output, look at what Element* we have, then add a breakpoint on RestyleTracker::AddPendingRestyle if aElement == thatPointer, and reverse-continue.)

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

From looking at profile timelines of loading few vs many tweets, I think the sibling restyles can be a problem.  The two large restyles dominate for small numbers of loaded tweets, but the sibling restyles seem to create a scalability problem for loading many tweets.

I'm testing on Windows at the moment.  So no rr.  This stuff is not hard to reproduce, though.

Comment 20

[Cameron McCormack (:heycam)]

OK.  For eRestyle_LaterSiblings, it would be interesting to know the exact selectors causing that to be generated.  We treat "+" and "~" somewhat similarly, in that when we expand out eRestyle_LaterSiblings we expand it out to all siblings, even for "+", when there really should only be a single later sibling that needs restyling.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

I added some debugging and it does appear that only the "+" operator is used on the timeline <li> elements.  The "~" is used on more constrained selectors that don't match the top level timeline elements.

Any thoughts on how to make "+" only restyle the next element?  I can try to figure something out, but any guidance is appreciated.  Thanks!

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll note that it's not as trivial as it might seem at first glance, since you need to handle "A:state + B + C" selectors, never mind "A:state + B ~ C".

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To expand on that, the current code only considers the combinator immediately to the right of the state or attribute.  You could switch to "restyle only one sibling", but you'd need to do so only when the combinator to the right of "+" is not "+" or "~".  (That's probably good enough.)

However, I'm not quite sure how you get there with the current data structures, since selectors are a singly-linked list from right to left, with the combinator attached to its left side exactly so that we can read it for the restyle optimizations in RestyleHintForOp.

So you'd need to do something in terms of getting the necessary information out of what we have in RuleCascadeData::mStateSelectors and RuleCascadeData::mAttributeSelectors, which are pointers into the middle of the selector linked list.

Comment 24

[Cameron McCormack (:heycam)]

RestyleHintForSelectorWithAttributeChange passes in the start of that list as aRightmostSelector, so at least in there you could loop from the right back to aSelector to see if there are any other "+" or "~" combinators.  Having to loop isn't great if we really only want to check one combinator to the right of aSelect, though we already do loop in one case:

https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/layout/style/nsCSSRuleProcessor.cpp#2847

It'd probably be fine to make nsCSSSelector a doubly-linked list.


Once you can detect this situation, you'll want to generate a new restyle hint, eRestyle_FollowingSibling or something, and then in RestyleTracker::ProcessOneRestyle expand it out to only the following one sibling element.  There are a bunch of eRestyle_LaterSiblings uses in Restyle{Tracker,Manager} that you'll need to consider and do something analagous with.

Also I think we'll want to avoid setting NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS in SelectorMatchesTree when we select past a "+" and we have the same condition of having a non-"+"-non-"~" selector just to the right.

Comment 25

[Cameron McCormack (:heycam)]

Another optimization which might or might not be worth doing is to determine whether the eRestyle_LaterSiblings / eRestyle_FollowingSibling really needs to be expanded into eRestyle_Subtree or whether eRestyle_Self is sufficient.  For example with:

  .a ~ .b .c

when you toggle the .a class you need to expand the eRestyle_LaterSiblings into eRestyle_Subtree because of the descendant combinator between .b and .c, but with:

  .a ~ .b

you only need to generate an eRestyle_Self for the following siblings.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the suggestions!

Right now I'm trying to just comment out the NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS here:

  https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#2406

I'm just curious if it will show any improvement or not before tackling the harder changes.

Unfortunately I'm not getting a high volume of tweets at the moment, so I need to wait until tomorrow to test.  Also, interestingly, the janky tweet load does not affect the search page.  Only the main twitter timeline.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Hmm, commenting out the NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS for "+"/"~" doesn't seem to have much impact here.

Comment 28

[Cameron McCormack (:heycam)]

Attachment: debugging

OK, so I think that means that insertion and removal of elements that might need elements to be restyled due to sibling combinators (which is why this flag is checked, in RestyleManager) isn't much of a problem with the site.

It would be interesting to know the entire selectors that end up generating these eRestyle_LaterSiblings hints, to confirm that a new eRestyle_FollowingSibling restyle hint would help.  Try this patch to dump out what we know about the selectors at the time we generate eRestyle_LaterSiblings, and match it up with the "adding pending restyle ... due to eRestyle_LaterSiblings" lines in the restyle log to see which selectors correspond to (a) the big restyle and (b) those many smaller restyles.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

FWIW, I tested against edge today as well.  On this particular load nightly completed in ~6 seconds while edge took ~16 seconds.

Also, I'm beginning to wonder if we are restyling more elements than chrome.  I see similar large restyles with many smaller follow-on restyles.  They just complete each one faster than we do.

I also noticed that our production PGO builds were noticeably faster than my local opt builds on this workload.  This suggests that maybe we just have higher overhead costs in our restyle system.

I was thinking of trying to convert our code from recursive function calling to a work stealing loop instead as a way to reduce execution overhead.

Comment 30

[Cameron McCormack (:heycam)]

(In reply to Ben Kelly [:bkelly] from comment #29)
> Also, I'm beginning to wonder if we are restyling more elements than chrome.
> I see similar large restyles with many smaller follow-on restyles.  They
> just complete each one faster than we do.

Ultimately I think that is the issue, that we end up restyling more elements that we need to.

> I also noticed that our production PGO builds were noticeably faster than my
> local opt builds on this workload.  This suggests that maybe we just have
> higher overhead costs in our restyle system.
> 
> I was thinking of trying to convert our code from recursive function calling
> to a work stealing loop instead as a way to reduce execution overhead.

There probably is some overhead of passing the same values down the stack (we create an ElementRestyler object on the stack for each recursive call), but I'm not convinced it's the dominant factor here.  I think we'll get better results from restyling fewer elements in response to the DOM modifications given the style rules we have here.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

I'm sorry.  I got busy with other things and I'm not currently working on this at the moment.  If someone wants to pick it up its pretty easy to reproduce.  You just need a twitter account that follows a lot of people.  Having people that talk to each other is probably slightly better to exercise conversation threading.

Comment 32

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

(In reply to Ben Kelly [:bkelly] from comment #31)
> I'm sorry.  I got busy with other things and I'm not currently working on
> this at the moment.  If someone wants to pick it up its pretty easy to
> reproduce.  You just need a twitter account that follows a lot of people. 
> Having people that talk to each other is probably slightly better to
> exercise conversation threading.

Hi :bkelly. Any suggestions for someone to pick this up? It's made it up the triaging and ranking with the platform-rel engineering managers for action on our end - and we have the DL with the Twitter team if required.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

IMO the best path forward is to implement stylo, which is already in progress.

Comment 34

[Jet Villegas (inactive)]

We'll want to have this become a static test (i.e, without twitter.com which changes daily) for Stylo. Adding the dependency...

Comment 35

[Gregory Pappas [:gregp]]

Seems to perform well.