fix(subject): Faster subscriptions management in Subject#7234
fix(subject): Faster subscriptions management in Subject#7234benlesh wants to merge 1 commit intoReactiveX:masterfrom
Conversation
The current implementation of the Subject was backing itself on an array of observers. When one observer unsubscribed itself, it was performing a linear scan on the array of observers and dropping the item from the array of observers. The algorithm complexity of this operation was O(n) with n being the number of observers connected to the Subject.
It caused code like:
```js
const allSubscriptions = [];
const source = new Subject();
for (let index = 0 ; index !== 1_000_000 ; ++index) {
allSubscriptions.push(source.subscribe()); // rather quick
}
for (const subscription of allSubscriptions) {
subscription.unsubscribe(); // taking very long
}
```
The proposed approach consists into changing our backing collection (an array) into a Map. But we lose somehow the set capability on subject.observers (might possibly be patched in some way).
Following that change the command `cross-env TS_NODE_PROJECT=tsconfig.mocha.json mocha --config spec/support/.mocharc.js "spec/**/Subje*-spec.ts"` passed to 452ms from 10s.
|
I like the idea here, but I immediately spotted that this breaks re-entrant subscriptions. To following will behave differently before and after this change: const subject = new Subject()
subject.subscribe(console.log)
subject.subscribe((value) => {
console.log(value);
subject.subscribe(console.log);
})This is because before we were protecting ourselves from re-entrant subscriptions modifying the observers we were looping over. I also spotted that we're removing an optimization we added that prevented us from needing to copy an Array over and over for this case. (That said, I also noticed that in that code we should be using In any case, @dubzzz's commit is an interesting idea, but unfortunately breaks a lot of our tests. We cannot accept it as is. I'll leave this open for now. And I'll think about it some more. |
Oups, sorry about that. I thought I launched then all. I'll re-run them locally to see what goes wrong in the coming days |
Will try to find a way to make it work with it.
EDITED: Actually when looking closer to CI run, I probably see what you mean by 'breaks a lot of our tests'. While the run is said ok, it has some errors (crashes due to OOM) I'll investigate them closer. |
|
Hi @benlesh, Not sure I can push on your PR 🤔 Maybe I can re-open one? Note: I have a fix for the re-entrant stuff. I'm checking performance of the new code with the pointers you sent me. |
|
I reopened it with everything fixed in theory. I also added benchmark results. See #7240 |
|
closed for #7240 |
2 participants
Since I HORRIBLY jacked up @dubzzz's PR at #7231 by attempting to use the GitHub Web IDE, I'm starting this new PR that rolls back to just @dubzzz's change.