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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃毐 Fix leak of LanguageMode objects in tests #19359

Merged
merged 3 commits into from May 18, 2019

Conversation

Projects
None yet
2 participants
@nathansobo
Copy link
Contributor

commented May 17, 2019

Fixes #18991

In #18991, we were experiencing render process crashes caused by the render process running out of memory. Upon investigating, we discovered that the memory used by the Atom Helper process rapidly increased during text-editor-spec.js, from a couple hundred megabytes to 3+ gigabytes by the end of those tests.

We were confused, because a heap snapshot of the process indicated a normal heap size of just a couple hundred megabytes. Upon further investigation, I discovered that we were leaking TextMateLanguageMode objects. These language modes reference TextMate grammars, which themselves reference a large number of Oniguruma regular expression objects, which are implemented in a native C++ and therefore not tracked as part of the JS heap. That explains the discrepancy between the process memory consumed and what was being reported in the Chrome dev tools.

The cause of the leak was subtle. In TextEditorRegistry, we were assigning a promise to an instance field named initialPackageActivationPromise. This would resolve when the PackageManager emitted an onDidActivateInitialPackages event. We then awaited this promise in updateAndMonitorEditorSettings, and this ended up implicitly capturing the oldLanguageMode variable that we interacted with after resuming from the await. Unfortunately, in many tests, we never emit the onDidActivateInitialPackages event at all, so this promise was hanging around unresolved. This was causing the oldLanguageMode to be kept in scope by the initialPackageActivationPromise we were awaiting. Not sure how clear that was, but hopefully it helps explain the diff to a careful reader.

My solution is to expose a getActivatePromise method on the PackageManager itself. We store off the promise during activation and then null it out when we're done. Most importantly, we also null out this promise when we call PackageManager.prototype.reset, so that any objects that are implicitly captured by code awaiting this promise get garbage collected.

As a result, we end text-editor-spec.js with 6 TextMateLanguageMode objects on the heap instead of 954, and the memory consumed by the process is kept more reasonable (though Jasmine still seems to be leakier than I'd prefer).

Before:

Screen Shot 2019-05-17 at 4 32 37 PM

After:

Screen Shot 2019-05-17 at 4 39 15 PM

@nathansobo nathansobo changed the title 馃毐Fix leak of LanguageMode objects in tests 馃毐 Fix leak of LanguageMode objects in tests May 17, 2019

@@ -674,6 +674,7 @@ module.exports = class PackageManager {
this.emitter.emit('did-activate-initial-packages')
this.activatePromise = null
})
return this.activatePromise

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 18, 2019

Author Contributor

Oops, thanks.

@nathansobo nathansobo merged commit e18559f into master May 18, 2019

2 checks passed

Atom Pull Requests #20190518.1 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nathansobo nathansobo deleted the ns/fix-language-mode-leak branch May 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.