search

moscajs / aedes-persistence

In-memory implementation of an Aedes persistence, with abstract tests
MIT License
13 stars 22 forks source link

chore: es5 to es6, drop nodejs 12 #82

Closed seriousme closed 2 years ago

seriousme commented 2 years ago

Items on the shopping list I hope to achieve in this PR:

All while maintaining the abstract interface, hence the two step approach so we can't accidentally mess up either of them ;-) If I missed anything just let me know. I created this as a draft PR so you can follow along.

Kind regards, Hans

seriousme commented 2 years ago

And as usual feedback is welcome. I'm Dutch so direct feedback is appreciated ;-)

lgtm-com[bot] commented 2 years ago

This pull request fixes 3 alerts when merging 7b5d638ca4f973c1c25b46fe5100c934a7d538ce into 883c249cac14168332e078f38037c605bcc18089 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request fixes 3 alerts when merging 382ea7cc28363b301af0b0d66d5a9fbfae68908d into 883c249cac14168332e078f38037c605bcc18089 - view on LGTM.com

fixed alerts:

seriousme commented 2 years ago

LGTM analysis highlighted the risk of prototype polution by using user data (e.g. clientID) as key to a plain object. Replacing plain objects by Maps fixes this.

lgtm-com[bot] commented 2 years ago

This pull request fixes 3 alerts when merging 2737104dc0effe0fc29f1ae5e9fd8f8ed91ebc24 into 883c249cac14168332e078f38037c605bcc18089 - view on LGTM.com

fixed alerts:

seriousme commented 2 years ago

Bumped into a suprise while testing abstract.js against the current version of this module. From2 seems to be dead, as it is based on readable-stream v2.x.x and was last updated when io.js was still a thing :-X

Going to try to work around this.

seriousme commented 2 years ago

Everything looks good so far. I'm still curious about how and if the iterators approach is faster or the same as using streams, could you try running some benchnamrks on aedes?

You can find them here: https://github.com/moscajs/aedes/tree/main/benchmarks

Of course you should use QoS > 1 and/or retained messages to test this

I'll have a go as soon as I fixed the From2 stuff ;-)

seriousme commented 2 years ago

The missing iterator on From2 streams should be fixable by using Readable().wrap(). From2 however seems not to be conformant to legacy streams spec either. E.g. : new Readable().wrap(From2stream) does create an iterator but does not produce data :-(

The following code:

const from2 = require('from2')
const { Readable } = require('stream')

function makeStream() {
    const queue = [{ a: 1 }, { b: 2 }, { c: 3 }]
    return from2.obj(function match(size, next) {
        let entry

        if ((entry = queue.shift()) != null) {
            setImmediate(next, null, entry)
            return
        }

        if (!entry) this.push(null)
    })
}

async function consume(stream) {
    console.log("Consume")
    for await (const value of stream) {
        console.log({ value })
    }
}

const legacy = makeStream()
consume(new Readable().wrap(legacy))

Should produce:

Consume
{ value: { a: 1 } }
{ value: { b: 2 } }
{ value: { c: 3 } }

but produces only:

Consume

So I'll need to do some more digging :-(

seriousme commented 2 years ago

Ok I found it:

This works :-)

const legacy = makeStream()
consume(new Readable({objectMode:true}).wrap(legacy))

Without {objectMode:true} the stream silently fails.

seriousme commented 2 years ago

Everything looks good so far. I'm still curious about how and if the iterators approach is faster or the same as using streams, could you try running some benchnamrks on aedes? You can find them here: https://github.com/moscajs/aedes/tree/main/benchmarks Of course you should use QoS > 1 and/or retained messages to test this

I'll have a go as soon as I fixed the From2 stuff ;-)

I will try to do some benchmarks soon. In the meantime, I found a blog article from nodesource with some relevant quotes: ;-)

It’s highly recommended to use async iterator when working with streams. According to Dr. Axel Rauschmayer, Asynchronous iteration is a protocol for retrieving the contents of a data container asynchronously (meaning the current “task” may be paused before retrieving an item). Also, it’s important to mention that the stream async iterator implementation use the ‘readable’ event inside.

You can also use async iterators to write to a writable stream, which is recommended

lgtm-com[bot] commented 2 years ago

This pull request fixes 3 alerts when merging c4c87dbe39cdc70de53f8f84b1fad02277073850 into 883c249cac14168332e078f38037c605bcc18089 - view on LGTM.com

fixed alerts:

seriousme commented 2 years ago

Although all checks passed I found some issue when trying to do the benchmarks. Please hold.

lgtm-com[bot] commented 2 years ago

This pull request fixes 3 alerts when merging 4ae378cd10da70cdaf6b8b0c16329ed7a40c7df3 into 883c249cac14168332e078f38037c605bcc18089 - view on LGTM.com

fixed alerts:

seriousme commented 2 years ago

I did some benchmarking using my laptop. As usual: benchmarking is an art! ( e.g. vscode was running in the background ) I used Node 14.19.1 with three shells:

  1. server.js
  2. throughputCounterQoS1.js
  3. bombingQoS1.js

Original: aedes-persistence@8.1.3 New: this PR

Original Sent/s New Sent/s Original Received/s New Received/s
6827,6 6340,4 62,2 0
8092,4 8315,8 6870,8 5865
8539 8645,2 8162,4 8248,6
8703,6 8637,4 8509,4 8663
8688,8 7227,6 8737,4 8774,2
8.566 8628,2 8710,8 7068
8836,6 8846,4 8601,2 8637,8
8697 8803 8770 8823
8170,6 8710 8626,6 8845
7934 9002,8 8148,8 8683,8
8921,2 8700 8003 9001,2
  8931,8 8905,6 8699,2
  7833,6   8917,8
  8254,2   7880,8
  8940,8   8221,2
      8943,6
Taking out the outliers at the start we end up with the following averages Original Sent/s New Sent/s Original Received/s New Received/s
8514,94 8534,06 8367,82 8529,09

Which seems to suggest imho that the generators are at least as good and maybe slightly better ;-)

Kind regards, Hans

seriousme commented 2 years ago

Btw: I also did the same benchmark on aedes-persistence-level ;-)

NodeJS: 14.19.1 Original: aedes-persistence-level@7.0.0 with level@7.0.0 New: aedes-persistence-level@8.0.0 with level@8.0.0

Original Sent/s New Sent/s Original Received/s New Received/s
5163,26 5738,36 5161,707692 5735,526316

Most of this will be due to perf improvements between Level@7 vs Level@8 though ;-)

lgtm-com[bot] commented 2 years ago

This pull request fixes 3 alerts when merging 73c50f8cbee987a4be2223d6b8bc94ed8eb3e563 into 883c249cac14168332e078f38037c605bcc18089 - view on LGTM.com

fixed alerts: