Optimize repeated field decoding

[ignatz]

Avoid allocating tear-off closures when decoding repeated but non-packed fields.

Also refactors BuilderInfo._decodeEnum.

Benchmarks

repeated_int64, repeated_string, and repeated_enum are the new benchmarks added with this PR.

fromBuffer is an internal benchmark that decodes a large buffer.

fromBufferUnmodifiableInput is the same as fromBuffer, but the input Uint8List is made unmodifiable.

AOT

	Before	After	Diff
repeated_int64	84,549 us	61,820 us	-22,729 us, -25.8%
repeated_string	67,183 us	67,984 us	+801 us, +1.19%
repeated_enum	52,335 us	44,772 us	-7,563 us, -14.4%
fromBuffer	25,661 us	25,299 us	-362 us, -1.4%
fromBufferUnmodifiableInput	26,135 us	25,689 us	-446 us, -1.7%

JS

	Before	After	Diff
repeated_int64	112,050 us	109,850 us	-2,200 us, -1.9%
repeated_string	123,050 us	126,650 us	+3,600 us, +2.9%
repeated_enum	43,920 us	39,400 us	-4,520 us, -10.2%
fromBuffer	211,888 us	168,250 us	-43,638 us, -20.5%
fromBufferUnmodifiableInput	227,444 us	185,454 us	-41,990 us, -18.4%

---

cl/609281182

[osa1]

Thanks, this is great. I've also confirmed in some internal benchmarks (not micro benchmarks) that this makes quite a big difference. I will try to review and merge this this week.

[osa1]

Thanks again, this is great.

While reviewing this I noticed that the wins here are not coming from the cached list, but from avoiding the tear-off closure creating when reading repeated fields, for every element.

Indeed, fs._ensureRepeatedField(...) is just a list access in the common case:

  PbList<T> _ensureRepeatedField<T>(BuilderInfo meta, FieldInfo<T> fi) {
    assert(!_isReadOnly);
    assert(fi.isRepeated);
    if (fi.index == null) {                             // <----- branch not taken in the common case
      return _ensureExtensions()._ensureRepeatedField(fi as Extension<T>);
    }
    final value = _getFieldOrNull(fi);                  // <----- returns non-null after the first element of a list
    if (value != null) return value;
    ...
  }

  dynamic _getFieldOrNull(FieldInfo fi) {
    if (fi.index != null) return _values[fi.index!];    // <----- common case
    return _extensions?._getFieldOrNull(fi as Extension<dynamic>);
  }

So it makes sense that it doesn't make too much difference. (actually it makes my benchmark slower when compiled to JS, but that may be noise, see below for numbers)

The wins here are coming from avoiding the closures in e.g.

      case PbFieldType._REPEATED_BOOL:
        _readPackable(meta, fs, input, wireType, fi, input.readBool);

Here for an unpacked list, we create one tear-off closure (input.readBool) for every element. That's expensive compared to the cost of reading one element. The version in this PR avoids it when the field is not packed:

      case PbFieldType._REPEATED_BOOL:
        final list = cachedList ?? fs._ensureRepeatedField(meta, fi);
        if (wireType == WIRETYPE_LENGTH_DELIMITED) {
          _readPacked(input, () => list.add(input.readBool()));
        } else {
          list.add(input.readBool());           // <----- no closure allocation when not packed
        }

(Allocating a closure when reading packed is fine, as closure allocation is probably a small part of the whole operation, though we could even avoid that if we wanted, similar to enum code in this PR, which doesn't allocate any closures)

We should update the commit message to reflect this. I can do this if you prefer.

Benchmark results

From an internal benchmark.

Master branch

AOT

fromBuffer(RunTime): 26,048 us.
fromBufferUnmodifiableInput(RunTime): 26,596 us.

JS

fromBuffer(RunTime): 214,111 us.
fromBufferUnmodifiableInput(RunTime): 230,777 us.

With cached list

AOT

fromBuffer(RunTime): 24,877 us.
fromBufferUnmodifiableInput(RunTime): 24,864us.

JS

fromBuffer(RunTime): 193,333 us.
fromBufferUnmodifiableInput(RunTime): 192,500 us.

Same patch, but without the cached list

AOT

fromBuffer(RunTime): 25,388 us.
fromBufferUnmodifiableInput(RunTime): 25,073 us.

JS

fromBuffer(RunTime): 171,272 us.
fromBufferUnmodifiableInput(RunTime): 189,000 us.

Notes

Cached list is not that important (actually makes dart2js output slower), the important part is avoiding creating a tear-off closure for every element when reading non-packed repeated fields.

Similar differences can be seen with the benchmark in this PR.

[ignatz]

We should update the commit message to reflect this. I can do this if you prefer.

Your findings match my findings. I never intended to claim that the caching has a big impact. My commit message specifically calls out "Boxing overhead", which is my, maybe inadequate lingo, to say I removed a closure.

I'm not sure you wanted to say that it's the allocation itself that is solely responsible for the overhead because I'm not sure that's true.

I left the cache from my experiments because it seemed fairly simple and not entirely pointless. It's not clear to me how it would slow things down unless this is some memory layout weirdness. We can remove it

Lastly, you should always feel free to edit my commit message

[osa1]

I'm testing this internally in cl/609281182.

[ignatz]

Awesome. Appreciate your changes. Thanks!