Page MenuHomePhabricator

Simplify internals of handlePending() and execute() to reduce stack depth
Closed, ResolvedPublic

Description

Problem

When inspecting a performance timeline (e.g. in Chrome), it initially appears as if none of the source code that we load on pages is actually executing. This is because the first many layers of the call stack are occupied by mw.loader internals.

Aside from burying actual module code deep down the graph, it also makes it hard to debug mw.loader itself, and to see how and where it spends time, because similar calls are not aligned with each other.

This means that vertically, all time spent in a particular entry point is fragmented between varying levels of recursion.

Details

The current way that one module executes after another makes for very difficult debugging and very deep and inconsistent stacks.

For example, here is a "normal" timeline of 19 simple script-only modules executing one after the other.

Which is effectively

execute() # module 1
 runScript() # module 1
  markModuleReady() #module 1
   handlePending()
    execute() # module 2
     runScript() # module 2
      markModuleReady() # module 2
       handlePending()
        execute() #module 3
          ..

and so on.

This would be much simpler to debug and understand the performance of, if after the end of execute(), it would resume the outer handlePending() rather than starting another one inside of it.

Outcome

  • The stack in which runScript() is called, should not start from "Animation frame fired" (requestAnimationFrame). Instead, it should start from an event (e.g. script response directly), or from requestIdleCallback.
  • The stack should not involve a runScript call inside another runScript call. Instead, when the next module is invoked, it should start again from the higher level in the stack, presumably the same layer that initiated the previous runScript.

Event Timeline

Krinkle created this task.Aug 24 2018, 3:22 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 24 2018, 3:22 AM
Krinkle updated the task description. (Show Details)Aug 26 2018, 3:47 AM
Krinkle updated the task description. (Show Details)Aug 26 2018, 3:57 AM

Change 455075 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Remove checkCssHandles for modules without styles

https://gerrit.wikimedia.org/r/455075

Change 455381 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Simplify addEmbeddedCSS by using object refs

https://gerrit.wikimedia.org/r/455381

Krinkle triaged this task as Low priority.Aug 26 2018, 4:19 AM
Krinkle moved this task from Inbox to Accepted: Enhancement on the MediaWiki-ResourceLoader board.

Change 455381 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Simplify addEmbeddedCSS by using object refs

https://gerrit.wikimedia.org/r/455381

Change 455075 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Remove checkCssHandles for modules without styles

https://gerrit.wikimedia.org/r/455075

Change 456426 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@wmf/1.32.0-wmf.19] resourceloader: Remove checkCssHandles for modules without styles

https://gerrit.wikimedia.org/r/456426

Change 456426 merged by jenkins-bot:
[mediawiki/core@wmf/1.32.0-wmf.19] resourceloader: Remove checkCssHandles for modules without styles

https://gerrit.wikimedia.org/r/456426

Change 456532 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Remove module stringification from execute path

https://gerrit.wikimedia.org/r/456532

Change 456532 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Remove module stringification from execute path

https://gerrit.wikimedia.org/r/456532

Change 457968 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@wmf/1.32.0-wmf.20] resourceloader: Remove module stringification from execute path

https://gerrit.wikimedia.org/r/457968

Change 457977 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@wmf/1.32.0-wmf.19] resourceloader: Remove module stringification from execute path

https://gerrit.wikimedia.org/r/457977

Change 457968 merged by jenkins-bot:
[mediawiki/core@wmf/1.32.0-wmf.20] resourceloader: Remove module stringification from execute path

https://gerrit.wikimedia.org/r/457968

Change 457977 merged by jenkins-bot:
[mediawiki/core@wmf/1.32.0-wmf.19] resourceloader: Remove module stringification from execute path

https://gerrit.wikimedia.org/r/457977

It seems our high stack depths actually causes a bug in DevTools FlameGraphs. Some kind of off-by-one error causing a single stack to be split up in to lots of chunk all with one less stack level depth, causing them to mis-align from the top top.

E.g. foo>bar>foo>bar>my_module calling A, B1>B2 and C1>C2>C3 renders as:

A < my_module < bar < foo < bar < foo
B2 < B1 < my_module < bar < foo < bar # missing one parent: foo
C3 < C2 < C1 < my_module < bar < foo # missing two parents: foo > bar

The result of this "trimming from the top" is that instead of creating a tree-like structure, each stack sample is its own unique column without a common path to get to it. So it is favouring details among the leaf nodes for accurate at the higher level. It seems to me like the opposite would be more useful, but either way, I don't think this was an intentional decision. Probably some kind of unforeseen circumstance.

Here are two examples of that happening. It typically creating a diagonal line of colour shifting that goes bends towards the top right.

These images make it look like some of these stacks were called multiple times, but actually aren't.

The top of the flame chart looks like this:

Change 459256 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Optimise and simplify state propagation logic

https://gerrit.wikimedia.org/r/459256

Krinkle claimed this task.Sep 8 2018, 11:44 PM
Krinkle raised the priority of this task from Low to Normal.

Prioritising a bit because of the unforeseen increase in mwLoadEnd that T192623 caused. Visual rendering times were not noticeably changed, but background execution of modules is taking longer.

That trade-off seems right, but I think this task might help recover the mwLoadEnd regression (and perhaps even do better than before) – whilst also allowing visual rendering times to improve (maybe). The current scheduling of execution is still too eager and ends up taking longer altogether with rendering.

Change 459256 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Optimise and simplify state propagation logic

https://gerrit.wikimedia.org/r/459256

Krinkle closed this task as Resolved.Sep 14 2018, 3:52 AM
Krinkle added a comment.EditedSep 15 2018, 1:34 AM

Moved to T192623.

Krinkle added a comment.EditedSep 15 2018, 2:02 AM

Moved to T192623.

Krinkle changed Risk Rating from N/A to default.
Krinkle moved this task from Untriaged to Archive on the Performance-Team-publish board.