Skip to content
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

Gamelab: add this.debuggerEnabled to avoid leak #25229

Merged
merged 1 commit into from
Oct 4, 2018

Conversation

cpirich
Copy link
Contributor

@cpirich cpirich commented Oct 4, 2018

  • Gamelab now maintains a this.debuggerEnabled state variable to remember whether or not it needs to worry about the debugger. Previously, it checked showDebugButtons || showDebugConsole only for calling jsDebugger.initialize() and potentially jsDebugger.open(). But, it would still call attach(), detach(), and appendLog() when it didn't really need to.
  • While this does makes things a tiny bit faster when there is no debugger around, the more important reason to make this change is that we were leaking a JSInterpreter instance for each Run/Reset when we did this, but only on Blockly gamelab levels. The Blockly instance holds out to a previous redux store state, that contains the previous JSInterpreter (even though we did properly null out the jsInterpreter state during jsDebugger.detach()). This may warrant further investigation, but the change above avoids the massive leak of JSInterpreter, p5, and everything else attached.
  • Heap snapshot reference trace:

screen shot 2018-10-04 at 12 55 37 am

@cpirich
Copy link
Contributor Author

cpirich commented Oct 4, 2018

@islemaster look good to you?

@islemaster
Copy link
Contributor

Sorry, I missed this yesterday, looking now.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Great find! I would not have guessed this. Do you recommend any particular change to our process that would keep us from accidentally re-introducing this leak?

@cpirich
Copy link
Contributor Author

cpirich commented Oct 4, 2018

I’m a little worried about what Blockly is doing and why the store is out of sync.

Additionally, I’m think we should avoid placing important objects like the JSInterpreter in the redux store.

As for how to catch it if we reintroduce it, maybe there is a way to run unit tests with heap snapshots afterward?

@cpirich cpirich merged commit dd09d74 into staging Oct 4, 2018
@cpirich cpirich deleted the check_debuggerEnabled_to_avoid_store_leak branch October 4, 2018 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants