Skip to content

Commit

Permalink
Revert code that caused a memory leak
Browse files Browse the repository at this point in the history
Summary:
As I was refactoring how we do code injection I noticed that the `this.instances` in
PJSOutput wasn't hooked up to anything so `if (this.instances) { this.instances.push(obj); }`
wasn't doing anything.  To "fix" this I added an `instances` array to the PJSOutput class object
and modified the `injectCode` instance method to use `PJSOutput.instances` instead.
Now that PJSOutput had an `instances` array, we were pushing every object ever created onto
that array.  I thought that reset the array whenever `injectCode` was called would handle
memory issues but I hadn't considered programs that new up object indefinitely.

This diff reverts these changes.  This means that all code having to do with `instances`
does nothing.  I'll remove this code in a following diff.

Test Plan: - test/output/pjs/index.html

Reviewers: pamela, john

Reviewed By: john

Differential Revision: https://phabricator.khanacademy.org/D25010
  • Loading branch information
kevinbarabash committed Jan 29, 2016
1 parent 77d5703 commit 0115512
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 14 deletions.
13 changes: 6 additions & 7 deletions build/js/live-editor.output_pjs.js
Expand Up @@ -650,7 +650,7 @@ var PJSCodeInjector = (function () {
this.grabObj = {};

// Extract a list of instances that were created using applyInstance
PJSOutput.instances = [];
this.instances = [];

// If we have a draw function then we need to do injection
// If we had a draw function then we still need to do injection
Expand Down Expand Up @@ -699,13 +699,13 @@ var PJSCodeInjector = (function () {

// Keep track of all the constructor functions that may
// have to be reinitialized
for (var i = 0, l = PJSOutput.instances.length; i < l; i++) {
constructors[PJSOutput.instances[i].constructor.__name] = true;
for (var i = 0, l = this.instances.length; i < l; i++) {
constructors[this.instances[i].constructor.__name] = true;
}

// The instantiated instances have changed, which means that
// we need to re-run everything.
if (this.oldInstances && PJSOutput.stringifyArray(this.oldInstances) !== PJSOutput.stringifyArray(PJSOutput.instances)) {
if (this.oldInstances && PJSOutput.stringifyArray(this.oldInstances) !== PJSOutput.stringifyArray(this.instances)) {
rerun = true;
}

Expand All @@ -727,8 +727,8 @@ var PJSCodeInjector = (function () {
}

// Reset the instances list
this.oldInstances = PJSOutput.instances;
PJSOutput.instances = [];
this.oldInstances = this.instances;
this.instances = [];

var _loop = function (i) {
// Reconstruction the function call
Expand Down Expand Up @@ -2962,7 +2962,6 @@ window.PJSOutput = Backbone.View.extend({

// Add in some static helper methods
_.extend(PJSOutput, {
instances: [],

// Turn a JavaScript object into a form that can be executed
// (Note: The form will not necessarily be able to pass a JSON linter)
Expand Down
12 changes: 6 additions & 6 deletions js/output/pjs/pjs-code-injector.js
Expand Up @@ -590,7 +590,7 @@ class PJSCodeInjector {
this.grabObj = {};

// Extract a list of instances that were created using applyInstance
PJSOutput.instances = [];
this.instances = [];

// If we have a draw function then we need to do injection
// If we had a draw function then we still need to do injection
Expand Down Expand Up @@ -642,15 +642,15 @@ class PJSCodeInjector {

// Keep track of all the constructor functions that may
// have to be reinitialized
for (let i = 0, l = PJSOutput.instances.length; i < l; i++) {
constructors[PJSOutput.instances[i].constructor.__name] = true;
for (let i = 0, l = this.instances.length; i < l; i++) {
constructors[this.instances[i].constructor.__name] = true;
}

// The instantiated instances have changed, which means that
// we need to re-run everything.
if (this.oldInstances &&
PJSOutput.stringifyArray(this.oldInstances) !==
PJSOutput.stringifyArray(PJSOutput.instances)) {
PJSOutput.stringifyArray(this.instances)) {
rerun = true;
}

Expand All @@ -672,8 +672,8 @@ class PJSCodeInjector {
}

// Reset the instances list
this.oldInstances = PJSOutput.instances;
PJSOutput.instances = [];
this.oldInstances = this.instances;
this.instances = [];

// Look for new top-level function calls to inject
for (let i = 0; i < fnCalls.length; i++) {
Expand Down
1 change: 0 additions & 1 deletion js/output/pjs/pjs-output.js
Expand Up @@ -497,7 +497,6 @@ window.PJSOutput = Backbone.View.extend({

// Add in some static helper methods
_.extend(PJSOutput, {
instances: [],

// Turn a JavaScript object into a form that can be executed
// (Note: The form will not necessarily be able to pass a JSON linter)
Expand Down

0 comments on commit 0115512

Please sign in to comment.