search

Polymer / shop

The Shop app
https://shop.polymer-project.org/
987 stars 494 forks source link

Memory leaks #95

Closed denar90 closed 7 years ago

denar90 commented 7 years ago

I was playing with perf investigations and found that going from home -> list/mens_outerwear -> home listeners and number nodes are increasing. (I can assume that this problem can be for other pages)

Number of listeners increased form 89 to 130, nodes 1881 -> 2532.

home - shop - timeline

Also I made a snapshot for current state and new one, repeating list/mens_outerwear -> home actions. It looks like there are a lot of detached nodes which have references to window.

home - shop - snapshot

P.S. Maybe before fixing this one it make sense to use drool and it's wrapper for polymer - polymer-drool for testing possible leaks. It requires add package.json to project (probably replacing bower).

ebidel commented 7 years ago

Here's another profile clicking between "Men's Outerwear" and hitting the back button. Nodes appeared to be GC'd but listeners are growing indefinitely.

screen shot 2016-11-02 at 9 15 59 am

Clicking between the "Men's Outerwear" tab and "Ladies Outwear" tabs:

screen shot 2016-11-02 at 9 18 14 am
keanulee commented 7 years ago

This may be related to how we use this.importHref on every _pageChanged - you can see that the <head> is filled with HTML imports for the same file, and each one of those is probably adding a listener for "load" (though at least the XHR request is not repeated since HTML imports are de-duplicate).

denar90 commented 7 years ago

@ebidel yeah plenty of leaks and @keanulee's solution might fix most of them. Other then that I propose to write some tests to find and fix other possible leaks.

frankiefu commented 7 years ago

There is a bug in Polymer.Base.importHref which it's not cleaning up the listeners (load/error) when it's trying to import the same file again. @blasten has a potential fix for this. He is also looking at the other issue which even on the same list page, switching between the "Men's Outerwear" tab and "Ladies Outwear" tab seems to cause the node count to grow.

blasten commented 7 years ago

This PR should fix the first issue: https://github.com/Polymer/polymer/pull/4124.

I was trying to repro the second issue: Clicking between the "Men's Outerwear" tab and "Ladies Outwear", and it seems like if you click the trash can to force GC, things drop to normal. cc @ebidel

ebidel commented 7 years ago

Yea, if I force GCs things drop (not completely), but reasonable:

screen shot 2016-11-02 at 5 37 36 pm
blasten commented 7 years ago

yeah. In my test, I went to https://shop.polymer-project.org/list/mens_outerwear and then clicked on ladies_outerwear. Then, I went back and forth between those two pages for a while and eventually stopped back on mens_outerwear, forced GC a few times, and went to ladies_outerwear one last time. In this test, I don't see a memory leak:

screen shot 2016-11-03 at 10 24 39 am

I think the key is to return to state from which the test started, and lastly press the trash can a few times.

denar90 commented 7 years ago

I recorded my profiling in incognito - https://monosnap.com/file/JDfs7ukxcQSO5cpjrNsRCgURfeKJam

Also here link to timeline-viawer with my profiling. It's not in incognito mode because of strange issue in Chrome, it's not saving timeline data in incognito, but other than that this result shows that issue with memory leak is still present. Maybe it can be helpful.

blasten commented 7 years ago

It might need an update. shop.polymer-project.org is running Polymer 1.4.0 and Polymer/polymer#4124 was fixed in 1.6.0 I believe.