Page MenuHomePhabricator

Prevent memory leaks from the nextState() function
Closed, DeclinedPublic

Description

The nextState() function is causing memory leaks. The nextState() is called on every link interaction many times. Each link interaction is causing the Popups extension to trigger "show popup logic", which triggers nextState() calls. Instead of using Object.assign() we decided to implemenet our own nextState() function because of lack of the IE support.

The main problem is that the state object contains not only scalar types, it also contains:

  • objects
    • baseData: {pageTitleSource: "PageA", namespaceIdSource: 0, pageIdSource: 5, isAnon: true, popupEnabled: true, …}
    • event: {action: "pageLoaded"}
    • interaction: {link: a, title: "PHP", namespaceId: 0, token: "e5db2f5fcd8e584869b1", started: 1543961297316, …}
  • jQuery events activeEvent: jQuery.Event {originalEvent: MouseEvent, type: "mouseover", isDefaultPrevented: ƒ, target: a, currentTarget: a, …}
  • dom nodes:
    • activeLink: a
    • link: a

I ran some tests with using our existing nextState() and with Object.assign() method. Object.assign() causes a smaller memory footprint. In general when Object.assign() was used Garbage Collector was able to free more memory.

For example, after ~3 minutes of interacting with Page previews (tests were executed multiple times):

nextState: JS memory heap increased by 0.8MB, ~1000 objects stored in memory
Object.assign(): JS memory heap increased by 0.5-0.6MB, ~200 objects stored in the memory.
Developer notes
  • ResourceLoader supports skipFunction which can be used to provide an Object.Assign() polyfill
  • We should not store DOM elements in the store, as those also might cause memory leaks (not properly disposed). Please verify is it possible to not store the activeLink or link in the state.
  • Popups' dependencies are quite outdated, some by major versions. It may be useful to try upgrading these to see if it affects the leak.

Event Timeline

pmiazga created this task.Dec 5 2018, 12:45 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 5 2018, 12:45 AM
Agabi10 added a subscriber: Agabi10.Dec 5 2018, 6:58 AM
ovasileva triaged this task as High priority.Dec 5 2018, 6:05 PM
Niedzielski updated the task description. (Show Details)Dec 5 2018, 7:12 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

A few quick thoughts:

Interesting find. Looking at the nextState function, it seems unlikely to leak, so I'd be interesting in figuring out what causes it. While a different method may avoid it in this case, it seems simple and generic enough to warrant better understand so that we may report the problem upstream and/or avoid the cause more generally (not just for the copy/assign use case).

In my own testing, I was unable to reproduce the leak (test case). I did notice a one-time increase, unrelated to how often the function was called. Which didn't happen with Object.assign. This suggests to me that there is some kind of lazy-initialised component in the JavaScript engine that is triggered by something in the function. Not sure what's going on there.

I would advise against using skipFunction for this purpose as the bytes spent on every page load for its module registration and skip data, and the run-time logic for running that function would exceed the size and time needed for the module itself. This would better suit local feature detection with fallback. If and when we end up having lots of these shims, in multiple extensions, before T178356 reaches its threshold, then we could temporarily group a number of them in a module like es6-shim, but I'd caution against premature optimisation/centralisation; these come at a cost.

My theory is that the two objects (nextState result and one of state or updates) reference each other in such a way that both objects retain one of their entries.
I'm trying to isolate this to the specific part of the object that is carrying the problem. I see state can have keys 'activeLink' and 'link' which point to a jQuery.Object OR activeEvent which is of type jQuery.Event

In terms of skipFunction that makes sense. The fallback could even be the existing memory leak version, give Object.assign availability is pretty high.. will continue digging.

pmiazga closed this task as Declined.Dec 19 2018, 1:36 AM

Looks like the object.assign() doesn't help as much as we thought it does.