Avoid leaking TextRow/BinaryRow object outside the framework

[testn]

Avoid leaking TextRow/BinaryRow object outside the framework by
returning a new object every time instead of returning this

[sidorares]

have you tried benchmarking it (select rows with bunch of fields and also access data in every returned field)
There might be potentially an impact on inline cache for object properties in case of generic object and object where constructor does this.propName = value

[sidorares]

Researching if it would be helpful to have per PR automatic benchmark action - do you have an experience with that @testn ?

Maybe https://github.com/benchmark-action/github-action-benchmark

[testn]

@sidorares

have you tried benchmarking it (select rows with bunch of fields and also access data in every returned field) There might be potentially an impact on inline cache for object properties in case of generic object and object where constructor does this.propName = value

it is quite tough to benchmark this on my machine somehow. The crude benchmark I tried has very high variance (+/-70%). Can you try this and see if you see any diff?

[testn]

re: accessing every field, sequelize currently copies every field from TextRow object into its own object. Therefore, every field is already accessed.

[sidorares]

I'll try to benchmark locally, also wonder how reliable numbers are when run inside GH action

[testn]

I actually tried to optimize this one a little further by creating it as a class instead where it will cache the typeCast field information for repeated use. The generated code will look something like this

(function anonymous(
) {
return ((function () {
  return class TextRow {
    constructor() {
      const _this = this;
      this.wrap0 = {
        type: "VAR_STRING",
        length: "24",
        db: "",
        table: "",
        name: "foo",
        string: function() {
          return _this.packet.readLengthCodedString("utf8");
        },
        buffer: function() {
          return _this.packet.readLengthCodedBuffer();
        },
        geometry: function() {
          return _this.packet.parseGeometryValue();
        },
        readNext: function() {
          return _this.packet.readLengthCodedString("utf8");
        }
      };
    }
    next(packet, fields, options) {
      this.packet = packet;
      const result = {};
      // "foo": VAR_STRING
      result["foo"] = options.typeCast(this.wrap0, this.wrap0.readNext);
      return result;
    }
  };
})())
})

[testn]

With this, typeCast query that returns 1,000 rows can yield up to 5-15% gain.

[sidorares]

FYI @testn - I added benchmark workflow in #1406 but as far as I understand the way it works it compares benchmarks from current run to the previous one and I see big variations in results even with no changes in the code. I'll see if I can change it ( instead of saving bench results in the cache for later access, always perform 2 benchmarks in the same run, one for master and one for current commit ). If performance alert comments become too noisy I'll disable them

[sidorares]

could you rebase your branch so it'll run same checks as in master? I removed travis and appveyor in favour of GH actions

[testn]

@sidorares Let me do that.

[testn]

@sidorares done