Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

Switching to Qlobber.Dedup #449

Closed
wants to merge 12 commits into from
Closed

Switching to Qlobber.Dedup #449

wants to merge 12 commits into from

Conversation

behrad
Copy link
Contributor

@behrad behrad commented Apr 14, 2016

This eliminates match/add pairs when adding topics to tries, and mosca is behaving more performant, Fixes #428, and I believe it also should fix #426

@mcollina
Copy link
Collaborator

Are you running this in production? Can you confirm it fixes #428 and maybe #426?

This is not passing on node v0.10 and v0.12, we should add a polyfill for those. I think Array.from is not supported.

@behrad
Copy link
Contributor Author

behrad commented Apr 14, 2016

Yes, It improves #428 , However there are still another optimizations available discussed on that topic 👍
I double checked #426 and it seems related to ascoltatori, May we introduce QlobberDedup in there?

Let me see what I can do for the Array.from for nodes < 4.0

@behrad
Copy link
Contributor Author

behrad commented Apr 14, 2016

Not to add a new npm dependency, I can use https://gist.github.com/brettz9/4212262, Where dya want me to put it @mcollina ?

@mcollina
Copy link
Collaborator

You can define it as a function somewhere, but don't polyfill array.

@behrad
Copy link
Contributor Author

behrad commented Apr 16, 2016

Why a fail? Can't see travis logs... :\

@behrad
Copy link
Contributor Author

behrad commented Apr 16, 2016

@davedoesdev What happens to QlobberDedup on node < 4 with no ES6 Set?

@davedoesdev
Copy link
Contributor

david@david-Latitude-E6440:~$ nvm use 0.12
Now using node v0.12.11 (npm v2.14.9)
david@david-Latitude-E6440:~$ node --version
v0.12.11
david@david-Latitude-E6440:~$ node
> Set
[Function: Set]

@behrad
Copy link
Contributor Author

behrad commented Apr 17, 2016

Mosca has 10, 12, 4 & 5: https://github.com/mcollina/mosca/blob/master/.travis.yml
there should be a problem on 0.10? or even with 5?

@davedoesdev
Copy link
Contributor

qlobber already polyfills Array.from: https://github.com/davedoesdev/qlobber/blob/master/test/dedup_spec.js#L26

@davedoesdev
Copy link
Contributor

QlobberDedup won't work on Node 0.10.
Node 5 will be fine.

@behrad
Copy link
Contributor Author

behrad commented Apr 17, 2016

QlobberDedup won't work on Node 0.10

Will drop 0.10 support @mcollina ?

@davedoesdev
Copy link
Contributor

Why does this PR need to use Array.from?

@behrad
Copy link
Contributor Author

behrad commented Apr 17, 2016

Seems async.eachdoesn't support ES6 Sets... you can find the code in commit log of this PR.
The problem seems to be ES6 which is not compatible with Mosca on 0.10

@davedoesdev
Copy link
Contributor

https://github.com/caolan/async#collections-1

Collection methods can iterate over Arrays, Objects, Maps, Sets, and any object that implements the ES2015 iterator protocol.

@behrad
Copy link
Contributor Author

behrad commented Apr 17, 2016

hmm... I should check why I was unable to iterate over them with async.each

@behrad
Copy link
Contributor Author

behrad commented Apr 22, 2016

I should check why I was unable to iterate over them with async.each

async 1.5.x doesn't support them, updated async to latest

behrad added a commit to adpdigital/mosca that referenced this pull request Apr 22, 2016
@davedoesdev
Copy link
Contributor

Hi @behrad is performance acceptable now or are there still problems?

@behrad
Copy link
Contributor Author

behrad commented Apr 28, 2016

Startup really improved @davedoesdev, I believe it was an effective change, Thank you for your nice and quick feedbacks.

However we have stills performance issues in Mosca with lots of connecting/disconnecting clients (~10/seconds)

I'm trying to check timeout management, sub trie add/removes and the way Mosca is working at last, in isolated tests.

@behrad
Copy link
Contributor Author

behrad commented Apr 29, 2016

I'm using this in my production, Will it be merged in @mcollina ?

@davedoesdev
Copy link
Contributor

@behrad FYI I just published qlobber 0.7.0, which now uses ES6 Maps internally for the trie. It seems to give roughly 25% speedup from the benchmarks.

@mcollina mcollina mentioned this pull request May 2, 2016
@behrad
Copy link
Contributor Author

behrad commented May 2, 2016

@behrad FYI I just published qlobber 0.7.0, which now uses ES6 Maps internally for the trie. It seems to give roughly 25% speedup from the benchmarks.

Is there any upgrade notes I should care about? Replaced Sets with Maps?

@davedoesdev
Copy link
Contributor

No - changes are all internal.

@behrad
Copy link
Contributor Author

behrad commented May 12, 2016

this is ready @mcollina

@mcollina
Copy link
Collaborator

Awesome, I will assemble a new major as soon as I have a bit of time.

@mcollina
Copy link
Collaborator

Already merged in v2, about to be released.

@mcollina mcollina closed this Jun 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants