LUAFDN-268 optimize createSignal memory usage #304
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
This all looks reasonable to me. Since this is a critical part of the codebase, I'd like some more eyes on it! @matthargett and @oltrep, what do you think?
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ | |||
| ## Unreleased Changes | |||
| * Added color schemes for documentation based on user preference ([#290](https://github.com/Roblox/roact/pull/290)). | |||
| * Fixed stack trace level when throwing an error in `createReconciler` ([#297](https://github.com/Roblox/roact/pull/297)). | |||
| * Optimized the memory usage of 'createSignal' implementation. | |||
There was a problem hiding this comment.
Can you add a link to the PR like the other items in the list?
| expect(spyA.callCount).to.equal(1) | ||
| end) | ||
|
|
||
| it("should have one connection instance when add the same listener multiple times", function() |
There was a problem hiding this comment.
can we have it warn the user when adding the same one multiple times? when this happens, it's probably the symptom of a deeper bug, right?
There was a problem hiding this comment.
For what it's worth, we try to be cautious about warnings when Roact powers things like in-game menu and studio plugins. React doesn't actually expose its signal implementation, so warning about this would signal an internal bug. If this cropped up in shipped code, we'd end up confusingly printing warnings to developers in studio about things that have nothing to do with them!
There was a problem hiding this comment.
I've added a warning message.
There was a problem hiding this comment.
Yeah I'm a bit worried about the warning too, I am not sure if that can bubble up to the consumers of Roact. I've checked out createContext and Binding and it seems ok to me
There was a problem hiding this comment.
I will remove it for now. We can loop back on the warning message in a separate PR if necessary
https://github.com/yjia2/roact into feature/LUAFDN-268-optimize-createSignal-memory-usage
| expect(spyA.callCount).to.equal(1) | ||
| end) | ||
|
|
||
| it("should have one connection instance when add the same listener multiple times", function() |
There was a problem hiding this comment.
Yeah I'm a bit worried about the warning too, I am not sure if that can bubble up to the consumers of Roact. I've checked out createContext and Binding and it seems ok to me
Closes LUAFDN-268.
DUAR has an "not enough memory" issue in production at createSignal. QA was able to reproduce it occasionally during logout. The memory consumption can spike as high as 3.4GB during logout. Inspected with on-screen profiler, it is the StyleConsumer unmount that are operating during the memory spike and createSignal.disconnect() takes the vast majority of the time. Consequently, we believe the createSignal is the root cause of the memory spikes during logout.
The current createSignal's copy-on-add algorithm is primarily for allowing adding new listeners during signal firing. But it is not memory efficient when handling a massive amount of listeners. Hence, the team decides to use an additional static table to hold the new listeners during firing instead of copying everything to an entirely new table.
Checklist before submitting:
CHANGELOG.md