Allow dart protos to be sent through sendports (and therefore to other isolates)

[mkustermann]

Decoded protos in Dart are represented in memory as instances of
GeneratedMessage subclasses. The GeneratedMessage stores the field
state in a _FieldSet and UnknownFieldSet.

The _FieldSet._meta field points to a BuilderInfo object, which
represents the metadata of the proto message.

Having this reference to BuilderInfo makes it hard to send dart
protos to other isolates, because

a) BuilderInfo hangs on to non-static closures, which cannot
be sent to to other isolates atm

b) Sending a transitive copy would copy the BuilderInfo object (and
it's transitive closure). That can cause big waste of memory
since an isolate would have this metadata possibly many times.

To avoid these two issues, this CL does the following:

• We replace _FieldSet._meta with a getter that accesses the
  BuilderInfo via the _FieldSet._message.info_ getter. This getter
  is implemented in GeneratedMessage subclasses and reads from a
  static field.

  => We effectively replace a load of the _meta field with load +
  interface call + static field load.

• To avoid performance regressions we avoid accessing the new
  _FieldSet._meta getter in the hot decoding loops. Instead we
  obtain this metadata once and pass it down to various methods that
  are part of decoding.

• For similar reasons, we remove the PbMap._entryBuilderInfo
  field - which also refers to a BuilderInfo. Instead we obtain the
  neccessary map-specific BuilderInfo in the main decoder loop.

This CL has to remove the 2 year long deprecated PbMap.add() function.

Closes #167

[sigurdm]

I had written something similar earlier - but gave up because it caused the benchmarks to get slower.

Did you try this in the golem benchmarks?

Edit: I see that you have gone out of your way to always have the BuilderInfo at hand instead of getting it. I guess that means the performance won't suffer.

[sigurdm]

LGTM

[mkustermann]

I had written something similar earlier - but gave up because it caused the benchmarks to get slower.

Yes, I started out with your change https://github.com/dart-lang/protobuf/pull/212/files. The main performance regression for that change was caused by calling the class _FieldSet { BuilderInfo get _meta => _message.info_; } getter inside the inner decoding loop. That means we replaced a load field with a load field + interface call + static field load for each loop iteration in the decoder.

I've tried to hoist this load of _FieldSet._meta out of the loop (which makes this such a big CL), similarly the original PR kept it for maps, for which I tried to do the same, hoisting it out until the BuilderInfo is no longer necessary in PbMap.

The only downside is that I had to remove this deprecated PbMap.add() method (IIRC)

Did you try this in the golem benchmarks?

After this hoisting it seemed to no longer cause big regressions.

[sigurdm]

The only downside is that I had to remove this deprecated PbMap.add() method (IIRC)

Not sure that's really a downside :)

[mkustermann]

@iinozemtsev Could you give this change a trial run in g3 to see if it causes any issues?

[iinozemtsev]

@mkustermann unfortunately we don't yet have automated copy process between g3 and github for protos. I can try porting this manually later today

[mkustermann]

@sigurdm made me aware that it might be better to base this off the null-safety branch, here's the PR: #452

[mkustermann]

Closing this PR since we only want critical bugfixes to land on master. Active development should happen on null-safety branch which will eventually become master.

NNBD PR: #452