Remove dynamic calls

[srawlins]

Work towards #578

Many of these fixes were guesses. Function f takes a dynamic which is later indexed with [] in a sub-path of the function. Function g passes a value to function f but that value is also dynamic. dynamic all the way down. The bright side is that these guesses are less guess-y than dynamic invocations. :)

I did not land any fixes in tests. This PR is to improve app size.

[srawlins]

I'm running an internal global presubmit, and will post back here.

[srawlins]

I am... super confused about that CI failure. Does not seem related...

[mraleph]

Can we write this code as

final listValue = fieldValue as List<Object?>;

and then use listValue? I find such code easier to read. This applies to all other cases.

(also I doubt it's always List<Int64>, though I am not sure).

[srawlins]

Done and switched to List<Object?>.

[srawlins]

I really like the new fieldValue as List<Object?>; pattern enabled by flow analysis / type promotion :) it feels strange to convert fieldValue as List<Object?>; to final listValue = fieldValue as List<Object?>;, because fieldValue is still promoted in such an assignment, so no errors pop up. The subsequent uses of fieldValue as a List<Object?> continue to work, and I must hunt down each reference to fieldValue, in the scope of the cast, and change it to refer to listValue.

[mraleph]

FWIW using a variable also allows to satisfy dart2js constraints, you could write

final List<Object?> listValue = fieldValue;

to promote at 0 cost for dart2js.

[rakudrama]

as checks incur a runtime cost on dart2js and are likely slower than the dynamic calls they replace. Please benchmark the changes for dart2js before committing.

One way to make the checks go away in production dart2js compiles is to use the fact that dart2js trusts implicit checks at -O3 and -O4.

This explicit check

      final messageMap = message._fieldSet._values[field.index!] as PbMap?;

can be made implicit like this:

      final PbMap? messageMap = message._fieldSet._values[field.index!] as dynamic;

[srawlins]

OK I've removed all explicit casts. I think the owners of this package should be making the tougher calls here.

[mraleph]

OK I've removed all explicit casts. I think the owners of this package should be making the tougher calls here.

Lets sit on this PR and return to it in a bit (~1 month). I don't have time to run benchmarks right now, but in a month we will have somebody on board who can take care of this.

[osa1]

Regarding x as T vs. T x_ = x as dynamic casts when targeting JS, here's a benchmark. In #670 I'm using jsontool. jsontool uses x as T-style casts in a very hot code: https://github.com/lrhn/json-tool/blob/37efd7644ca4688aa0694aeb3693f3a773eba865/lib/src/json/sink/object_writer.dart#L41-L44

With the current version of the library these are the results I'm getting in my benchmarks:

ToProto3JsonObject(RunTime): 38961.53846153846 us.
ToProto3JsonObject(RunTime): 38865.38461538462 us.
ToProto3JsonObject(RunTime): 39705.882352941175 us.

If I replace those casts to T x_ = x as dynamic-style casts with this patch:

diff --git a/lib/src/json/sink/object_writer.dart b/lib/src/json/sink/object_writer.dart
index af5b562..2dab549 100644
--- a/lib/src/json/sink/object_writer.dart
+++ b/lib/src/json/sink/object_writer.dart
@@ -38,10 +38,12 @@ class JsonObjectWriter implements JsonWriter<Object?> {
     } else {
       var top = _stack.last;
       if (_key != null) {
-        (top as Map<String?, dynamic>)[_key] = value;
+        Map<String?, dynamic> map = top as dynamic;
+        map[_key] = value;
         _key = null;
       } else {
-        (top as List<dynamic>).add(value);
+        List<dynamic> list = top as dynamic;
+        list.add(value);
       }
     }
   }

These are the results I get:

ToProto3JsonObject(RunTime): 19259.615384615383 us.
ToProto3JsonObject(RunTime): 19417.47572815534 us.
ToProto3JsonObject(RunTime): 19861.38613861386 us.

It literally halves the runtime!

[osa1]

After realizing how costly type casts are when targeting JS (see my comment above), I re-implemented this PR myself in osa1@1dcf951.

Results: (copied from commit message)

File size:

	Before	After	Diff
JS	348,393 bytes	344,804 bytes	-3,589 bytes -1.0%
AOT	7,235,848 bytes	7,206,256 bytes	-29,592 bytes -0.4%

JS runtime:

	Before	After	Diff
FromBinary	43,361 us	39,588 us	-3,773 us, -8.7%
ToBinary	37,796 us	36,636 us	-1,160 us, -3.0%
FromJson	47,465 us	43,085 us	-4,380 us, -9.2%
ToJson	75,185 us	70,689 us	-4,496 us, -5.9%
FromProto3JsonString	37,555 us	34,500 us	-3,055 us, -8.1%
ToProto3JsonString	76,259 us	70,068 us	-6,191 us, -8.1%
FromProto3JsonObject	17,213 us	17,025 us	-188 us, -1.0%
ToProto3JsonObject	21,836 us	19,607 us	-2,229 us, -10.2%
HashCode	12,151 us	12,029 us	-122 us, -1.0%

AOT runtime:

	Before	After	Diff
FromBinary	6,428 us	6,581 us	+153 us, +2.3%
ToBinary	12,560 us	12,893 us	+333 us, +2.6%
FromJson	27,877 us	28,274 us	+397 us, +1.4%
ToJson	34,721 us	36,316 us	+1595 us, +4.5%
FromProto3JsonString	31,825 us	32,587 us	+752 us, +2.3%
ToProto3JsonString	40,089 us	39,135 us	-954 us, -2.3%
FromProto3JsonObject	9,002 us	9,091 us	+91 us, +0.98%
ToProto3JsonObject	11,163 us	10,925 us	-238 us, -2.1%
HashCode	5,108 us	5,007 us	-101 us, -1.9%

---

Avoiding dynamic calls and explicit type casts is a big win on JS.

However on native it's slower on most benchmarks.

It's unclear to me whether we want to make that trade.

To me the fundamental problem is that JS and native targets are wildly different, and what is an optimization for one can easily be a pessimization in other. The only solution that I can see is conditional compilation, which is difficult to do in Dart, and too much conditional compilation also makes testing more difficult and code unmaintainable.

I'm just hoping that Wasm will be mature enough soon (the spec, implementations in browsers, dart2wasm) and we will be able to target Wasm instead of JS..

[srawlins]

I don't really have any horse in this race. I opened this PR for @apwilson who was trying to remove dynamic calls in order to reduce the build size of a Flutter app (not to improve runtime performance). But feel free to close if the tradeoff is not worth it.

[osa1]

I was trying to understand why AOT results are worse in my comment above, and I realized that results are noisy and even when I compare master-to-master using the two work dirs that I have, one of them generate much worse results.

I will try to figure out how to run these benchmarks more reliably and update the numbers.

[osa1]

I can't update this PR so created #685.