Popups: [Spike, 16hrs] Replicate Popups memory leak without other extensions
Open, HighPublic0 Story Points

Description

A memory leak has been detected in the Popups extension.
The purpose of this spike is to isolate it further by reducing noise from other extensions.

Outcomes of the spike

  • Someone on Reading web team is able to replicate the memory leak on a production page via the detailed notes in https://phabricator.wikimedia.org/T205127#4675717
  • The same memory leak has been replicated on a localhost/staging environment with minimal setup (to isolate leaks from other extensions)
  • The origin of the memory leak has been located. Ideally we should know exactly what is causing this, but at minimum let's aim to understand which module/part of the feature is at fault.
  • Is the memory leak OS specific or for all browsers?

Output

  • This ticket has been closed with an actionable next step to remove the memory leak. Note: Fixing the memory leak is not necessarily in scope for this task.
Jdlrobson triaged this task as Normal priority.
Jdlrobson raised the priority of this task from Normal to High.
Jdlrobson moved this task from To Triage to Upcoming on the Readers-Web-Backlog board.
Tbayer added a subscriber: Tbayer.Nov 2 2018, 11:54 PM
Jdlrobson lowered the priority of this task from High to Normal.Nov 6 2018, 5:37 PM

Reminder for @Jdlrobson: This doesn't seem to be assigned to anyone yet.

Jdlrobson added a comment.EditedMon, Nov 12, 10:33 PM

@Whatamidoing-WMF That's because nobody is actively working on it right now. A time-sensitive A/B test (T209306) is draining the entire team's time (sorry about this!).

Jdlrobson renamed this task from [Spike, 16hrs] Replicate Popups memory leak without other extensions to Popups: [Spike, 16hrs] Replicate Popups memory leak without other extensions.Thu, Nov 15, 6:48 PM
Jdlrobson set the point value for this task to 0.Wed, Nov 21, 5:33 PM
JFG added a comment.Mon, Nov 26, 8:58 PM

Any news? Can I help in any way?

ovasileva raised the priority of this task from Normal to High.Tue, Nov 27, 5:05 PM

I'm still digging into the code. It looks like there is a memory leak (I don't want to confirm it yet. First I need to understand what's going on.). Most probably the leak is caused by hover events when Popups and EventLogging are enabled.
I also noticed that after removing PagePreviews MediaWiki is still leaking (not that much but still).-

^ and to clarify this is on a VANILLA instance (e.g. little to no extensions) \o/ some progress....

We think we've isolated this problem to https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/reducers/nextState.js
We notice that using Object.assign here makes 90% of the memory leaks go away. I believe this relates to using Redux with mutable states rather than the immutable ones it was intended to. We also think we've found a small memory leak in VisualEditor @pmiazga will follow up with some actionables early next week once he has time to write up his findings.

I can replicate memory leak, and honestly, it leaks a lot. Good part is that we do not leak HTML DOM nodes, most of the allocated (and not freed up) memory is code/closures/variables/arrays/objects. I was testing most of the stuff using local vagrant environment with only Popups and EventLogging extensions. Popups were configured to use restbaseHTML gateway - pointing to the production wiki.

I was able to narrow searching to couple elements:

  • nextState() is copying over lots of elements (objects/arrays/DOM nodes), changing nextState to object.assign() reduces the leak (although still some memory is allocated)
  • wait() is called very, very often (it returns a promise that resolves after some time). I'm not sure if this is causing a memory leak, but definitely, something that we can verify as the code is primarily calling that function many times on each link interaction
  • when I remove all related tracking (statsvTracker, eventLoggingTracker, pageviewTracker) the memory footprint is lower, but that's most probably because the mw.track keeps a history of all events.

To solve the nextState() issue, I made T211168. Whenever I have time, I'll keep playing with the Memory tab and I'll try to find more memory leaks. Every day I learn something new about how to use Chromium profiling tools.

environment with only Popups and EventLogging extensions. Popups were configured to use restbaseHTML gateway

Nice detail!

Chromium profiling tools.

Is the nextState() leak identified reproducible in Firefox?