skale-me / skale

High performance distributed data processing engine
https://skale-me.github.io/skale
Apache License 2.0
399 stars 53 forks source link

for loop over items used later as index #178

Closed vsimko closed 6 years ago

vsimko commented 7 years ago

https://github.com/skale-me/skale-engine/blob/41f4a09c5d254f5d3b1b8598efaeba5aecb69464/lib/client.js#L339-L340

I just found this for-loop which seems to me as a bug. You are using the variable i as an index to dev, but the i here contains the item itself (or am I missing something?)

Therefore, instead of:

for (var i in dev)
      self.hostId[dev[i].uuid] = dev[i].id;

I would write:

for (const d in dev) {
      self.hostId[d.uuid] = d.id;
}
mvertes commented 7 years ago

There is no bug, because dev is really an Array, such as:

dev = [ { id: 1, uuid: 'a5234' }, { id: 2, uuid: 'b72322' } ];

for  (var i in dev) console.log(i, dev[i], dev[i].id);
// 0 { id: 1, uuid: 'a5234' } 1
// 1 { id: 2, uuid: 'b72322' } 2
// Good

for (const d in dev) console.log(d, d.id, d.uuid);
// 0 undefined undefined
// 1 undefined undefined
// Bad

Although, I agree the code is not clear, nor optimal, and can be improved. It should be better to iterate over numerical rather than string indices, and adopt ECMA 5.1 syntax:

for (let i = 0; i < dev.length; i++) console.log(i, dev[i], dev[i].id);
// 0 { id: 1, uuid: 'a5234' } 1
// 1 { id: 2, uuid: 'b72322' } 2
vsimko commented 7 years ago

What about using the ES6 syntax for..of ?

for (let d of dev) console.log(d, d.id, d.uuid);
// {id: 1, uuid: "a5234"} 1 "a5234"
// {id: 2, uuid: "b72322"} 2 "b72322"
// Good
mvertes commented 7 years ago

for .. of is certainly more elegant, but unfortunately, also much slower than good old for. I would stick to classical for loop at this moment.

micro benchmark:

#!/usr/bin/env node

var Benchmark = require('benchmark');

// Initialize an array of objects
var arr = [];
for (var i = 0; i < 128; i++) arr.push({key: "key_" + i, value: i});

var arr2 = [];

function for_loop() {
    var res = 0;
    for (var i = 0; i < arr.length; i++)
      res += arr[i].value;
    return res;
}

function forof_loop() {
    var res = 0;
    for (let item of arr)
      res += item.value;
    return res;
}

Benchmark.Suite().
  add('for_loop', for_loop).
  add('forof_loop', forof_loop).
  on('cycle', function(event) {
    console.log(String(event.target));
  }).
  on('complete', function() {
    console.log('Fastest is ' + this.filter('fastest').map('name'));
  }).
  run();

Results on my laptop:

for_loop x 3,523,368 ops/sec ±1.40% (86 runs sampled)
forof_loop x 2,367,000 ops/sec ±2.18% (84 runs sampled)
Fastest is for_loop