New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory Leak Fixes #1714

Merged
merged 3 commits into from Jun 5, 2015

Conversation

Projects
None yet
2 participants
@samccone

samccone commented Jun 5, 2015

This PR contains two memory leak fixes that quickly get out of hand on large notebooks.


Leak 1

codecell directive codemirror binding leak ⛲️

Steps to reproduce:
  • Start a profiler with an empty notebook
  • create a html cell and delete it (3 times)
  • force a GC
  • check the number of nodes and event listeners still in memory
Before

screen shot 2015-06-05 at 9 39 26 am

Note how the yellow and green lines (denoting event listeners and DOM nodes never goes down)

At this point I figured we were leaking something, so I took a look at the codecell directive and found what I was looking for:

scope.cm.on('change', changeHandler);

We were watching for the change event on the codemirror instance and calling a function inside of our directive, thus making the codemirror instance impossible to GC since it was always in the graph.

This was a simple fix that just required us to explicitly unregister for the change event on $scope.$destroy of the directive.

After

screen shot 2015-06-05 at 9 57 35 am

Awesome one leak down!


Leak 2

markdown editable directive leaks ⛲️

After using the app for a while I had noticed that typing in section cells can get quite slow. I hypothesized that perhaps section cells were bleeding memory and were not being cleaned up correctly. So from that initial itch I proceeded.

Steps to reproduce:
  • Start a profiler with an empty notebook
  • add a new section
    • click off of the section area to unfocus the editor
    • delete the section
  • (repeat above 3x)
  • force a GC and stop the profiler
Before

screen shot 2015-06-05 at 11 28 56 am

Owch ok, (I know there is no scale here, but this was leaking over ~1000 DOM nodes per new section cell)

From this I started digging down into the directive tree of section cell to markdown editable cell.
After reading over the markdown editable source code I fairly quickly was able to identify some issues in the source code. There was a combination of listening directly to the codemirror instance as in leak 1 as well as a new kind of leak caused by the way that we were registering codemirror instances back up to the notebook.

⚠️

scope.cm.on("blur", function(){
  scope.$apply(function() {
    syncContentAndPreview();
  });
});

⚠️ ⚠️ 🔥 💀

scope.bkNotebook.registerFocusable(scope.cellmodel.id, scope);
scope.bkNotebook.registerCM(scope.cellmodel.id, scope.cm);

The fix here was straightforward, we just needed to listen to $scope.$destroy and unregister our events to disconnect this content from the memory graph.

After

screen shot 2015-06-05 at 12 03 17 pm

💃 fixed!


Process takeaways

In the devtools resources they recommend to use the heap snapshots and heap timelines to identify and find leaking items (and be able to match the leaking objects to objects created in your app). Unfortunately past their "simple" examples that they have put together in the documentation I find this method of debugging to be incredibly difficult. On a complex application with many objects moving in and out of memory, these unrelated objects cloud the snapshots so much that identifying the object(s) that are leaking becomes basically impossible.

It may just be me, however after spending quite a bit of time digging through these dumps, I had no good takeaways:

screen shot 2015-06-05 at 1 52 33 pm
screen shot 2015-06-05 at 1 52 14 pm
screen shot 2015-06-05 at 1 51 47 pm

The heap timeline does show me that something is leaking, but past that I need to dig into the source code (of my application) to actually learn more.


In summary

The most powerful process that I have found so far is to follow these steps.

  • Draw a hypotheses based on your application domain experience.
  • Figure out a testing methodology that is easily repeatable
  • Use the timeline profile and forced GCs to then execute your test.

scottdraves added a commit that referenced this pull request Jun 5, 2015

@scottdraves scottdraves merged commit 662e537 into master Jun 5, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@samccone samccone deleted the sjs/leak-work branch Jun 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment