Don't we need something better than Promise.all?

Question

Here is my pretty code using await/async

monthlyBuckets(req, res) {
  const monthlyBuckets = []
  const now = DateTime.local()
  let date = config.beginningOfTime
  while (date < now) {
    monthlyBuckets.push({
      epoch: date.toMillis(),
      month: date.month,
      year: date.year,
      actions: await redis.get(`actions_monthly_${date.year}_${date.month}`),
      interested: await redis.scard(`sinterested_monthly_${date.year}_${date.month}`),
      adventurous: await redis.scard(`sadventurous_monthly_${date.year}_${date.month}`),
      active: await redis.scard(`sactive_monthly_${date.year}_${date.month}`),
    })
    date = date.plus({month: 1})
  }
  res.status(200).json(monthlyBuckets)
}

I like it, but making so many requests not in parallel leads to a request time close to 3 sec.

So, here is my ugly solution without async/await, just promises:

monthlyBuckets(req, res) {
    const monthlyBuckets = []
    const actions = []
    const interested = []
    const adventurous = []
    const active = []
    const now = DateTime.local()
    let date = config.beginningOfTime
    let entryCount = 0
    while (date < now) {
      monthlyBuckets.push({
        epoch: date.toMillis(),
        month: date.month,
        year: date.year,
      })
      actions.push(redis.get(`actions_monthly_${date.year}_${date.month}`))
      interested.push(redis.scard(`sinterested_monthly_${date.year}_${date.month}`))
      adventurous.push(redis.scard(`sadventurous_monthly_${date.year}_${date.month}`))
      active.push(redis.scard(`sactive_monthly_${date.year}_${date.month}`))
      date = date.plus({month: 1})
      entryCount++
    }
    const data = await Promise.all(actions.concat(interested).concat(adventurous).concat(active))
    for (let i = 0; i < entryCount; i++) {
      monthlyBuckets[i].actions = data[i]
      monthlyBuckets[i].interested = data[entryCount + i]
      monthlyBuckets[i].adventurous = data[entryCount * 2 + i]
      monthlyBuckets[i].active = data[entryCount * 3 + i]
    }
    res.status(200).json(monthlyBuckets)
  }
}

That ain't pretty, but it gets the job done under 200ms

Can I have pretty and efficient?

You can use `await Promise.all(...)` just fine. – Stephen Cleary Jan 26 '19 at 14:28 — Stephen Cleary, Jan 26 '19 at 14:28
That makes it 2% less ugly – standup75 Jan 27 '19 at 17:52 — standup75, Jan 27 '19 at 17:52

Jannes Botis · Accepted Answer · 2019-03-07T17:55:39.247

The issue with the code above is that you are trying to:

use one Promise.all() for all the promises
process the output of all the responses in one callback

Though this is not a mistake, it can lead to hard to "read" code.

The code could be written as:

while (date < now) {
  let dateData = {
    epoch: date.toMillis(),
    month: date.month,
    year: date.year,
  };
  let promiseData = Promise.all([
      dateData, // dataData is cast(made to) automatically into a promise
      redis.get(`actions_monthly_${date.year}_${date.month}`),
      redis.scard(`sinterested_monthly_${date.year}_${date.month}`),
      redis.scard(`sadventurous_monthly_${date.year}_${date.month}`),
      redis.scard(`sactive_monthly_${date.year}_${date.month}`)
  ]).then([data, actions, interested, adventurous, active] => {
      // process the data here for each month
      data.actions = actions;
      data.interested = interested;
      data.adventurous = adventurous;
      data.active = active;
      return data;
});
  monthlyBuckets.push(promiseData);
  date = date.plus({month: 1});
}

const data = await Promise.all(monthlyBuckets);
res.status(200).json(data);

What changed is

Grouping the promises for each month
Processing a month `s group of promises, not all the promises together and returning the data as wanted.

There is nothing wrong with grouping promises eg:

Promise.all([
       Promise.all([ ...]),
       Promise.all([ ...]),
       singlePromise,
       ...
]);

processing promises eg:

promiseProcessed1 = promise1.then(callback1);
promiseProcessed12 = Promise.all([promise1, promise2]).then(callback2);

or reusing promises eg:

promiseProcessed1 = promise1.then(callback1);
promiseProcessed12 = Promise.all([promise1, promise2]).then(callback2);
resultDatapromise = Promise.all([promise1, promise2, promiseProcessed1, promiseProcessed12]).then(callback2);

References

This is slightly easier to read. But will run slower, it is not retrieving the data for the next month until the previous month has been processed, but there is no real reason to do so. The array argument returned in Promise.all().then is not on par with the general esthetic of ES6 syntax. That is what prompted this question. I think ES6 promises is a good start, but I realize it has a lot to be improved upon. — standup75, Feb 28 '19 at 22:37
It will not run slower and "not retrieving the data for the next month until the previous month has been processed" is not true. As for "general esthetic of ES6 syntax", maybe you can use "destructuring in the callback arguments": eg like in the answers here: https://stackoverflow.com/questions/42750254/promise-all-spread-is-not-a-function-when-running-promises-in-parallel — Jannes Botis, Feb 28 '19 at 22:54
That's right, there is no await in your code. Yes could probably write ...then([actions, interested, adventurous, active] => ({actions, interested, adventurous, active})) to avoid indexed arrays — standup75, Feb 28 '19 at 23:18
`const data = await Promise.all(monthlyBuckets);` this line is superfluous — , Mar 07 '19 at 13:12
@JonathanR thanks for pointing this out, you are right. Updated the code. — Jannes Botis, Mar 07 '19 at 17:56

score 0 · Answer 2 · answered Mar 05 '19 at 07:06

Taking apart different steps could help in this situation. Example:

function createBucket(date, ops){
    const b = {
        epoch: date.toMillis(),
        month: date.month,
        year: date.year,
        actions: redis.get(`actions_monthly_${date.year}_${date.month}`),
        interested: redis.scard(`sinterested_monthly_${date.year}_${date.month}`),
        adventurous: redis.scard(`sadventurous_monthly_${date.year}_${date.month}`),
        active: redis.scard(`sactive_monthly_${date.year}_${date.month}`),
    }

    const promised = ['actions','interested', 'adventurous', 'active'];
    promised.forEach(p => ops.push(async () => {b[p] = await b[p]}));
}

async function monthlyBuckets(req,res){
    const monthlyBuckets = []
    const now = DateTime.local()
    let date = config.beginningOfTime

    const ops = [];
    while (date < now) {
      monthlyBuckets.push(createBucket(date,ops));
      date = date.plus({month: 1})
    }

    await Promise.all(ops);
    res.status(200).json(monthlyBuckets)
}

Don't we need something better than Promise.all?

2 Answers2