Closed jinyus closed 7 months ago
Hey @jinyus , thanks for your contribution. TBH, I'm a bit on the fence with this one. It looks reasonable to me, but I'm not sure if it can realistically yield some measurable performance improvement. I think it would be great to measure the performance impact of this change first.
Also, could you please elaborate on the "safer" part? What is unsafe about the current approach?
While this might not materialize in the real world, using static dispatch is 30x faster. You also get the benefit of type safety and the guarantee that it'll work forever. Did you test in wasm environment?
Dynamic dispatch(RunTime): 0.54397875 us.
Static dispatch(RunTime): 0.01802021385583829 us.
import 'dart:async';
import 'package:benchmark_harness/benchmark_harness.dart';
class Disposable {
var disposed = false;
void dispose() {
disposed = true;
}
}
class DynamicDispatchBenchmark extends BenchmarkBase {
final dynamic disposable;
DynamicDispatchBenchmark(this.disposable) : super('Dynamic dispatch');
@override
void run() {
_tryDispose(disposable);
}
}
class StaticDispatchBenchmark extends BenchmarkBase {
final dynamic disposable;
StaticDispatchBenchmark(this.disposable) : super('Static dispatch');
@override
void run() {
if (disposable case final Disposable d) {
d.dispose();
}
}
}
void main() {
// Create a disposable object
final disposable = Disposable();
final disposable2 = Disposable();
// Run the benchmarks
final dynamicBenchmark = DynamicDispatchBenchmark(disposable);
dynamicBenchmark.report();
final staticBenchmark = StaticDispatchBenchmark(disposable2);
staticBenchmark.report();
}
void _tryDispose(dynamic obj) {
runZonedGuarded(
() => obj.dispose(),
(error, stack) {
if (error is NoSuchMethodError) {
return;
}
final errorStr = error.toString();
if (!errorStr.contains('dispose\$0 is not a function') &&
!errorStr.contains('has no instance method \'dispose\'')) {
throw error;
}
},
);
}
using static dispatch is 30x faster
I'm not seeing that in your benchmark. I see the cost of running the dynamic.dispose()
inside a custom Zone
. In fact, modifying the provided benchmark to not_create a Zone
around a dynamic.dispatch()
call, we'll see that there's no measurable performance benefit of the type being known statically prior to the call:
class Disposable {
var disposed = false;
void dispose() {
disposed = true;
}
}
class DynamicDispatchBenchmark extends BenchmarkBase {
final dynamic disposable;
DynamicDispatchBenchmark(this.disposable) : super('Dynamic dispatch');
@override
void run() {
disposable.dispose();
}
}
class StaticDispatchBenchmark extends BenchmarkBase {
final dynamic disposable;
StaticDispatchBenchmark(this.disposable) : super('Static dispatch');
@override
void run() {
if (disposable case final Disposable d) {
d.dispose();
}
}
}
void main() {
// Create a disposable object
final disposable = Disposable();
final disposable2 = Disposable();
// Run the benchmarks
// Comment out these two lines to test "static dispatch"
final dynamicBenchmark = DynamicDispatchBenchmark(disposable);
dynamicBenchmark.report();
// Comment out these two lines to test "dynamic dispatch"
final staticBenchmark = StaticDispatchBenchmark(disposable2);
staticBenchmark.report();
}
yields the following results on my M1 MacBook:
Dynamic dispatch(RunTime): 0.006216865660449801 us.
Static dispatch(RunTime): 0.006216912327116421 us.
So, the performance gain can be obtained only from removing the custom Zone
creation around each dispose
call, which your PR does.
Logically, I thought that maybe reusing the Zone
for a "dispose queue" will work, but it turned out to be even worse for performance than wrapping every dynamic.dispose()
call in its own Zone
:
import 'dart:async';
import 'package:benchmark_harness/benchmark_harness.dart';
final _disposeQueue = StreamController<dynamic>(sync: true);
void _runDisposeQueue() {
runZonedGuarded(
() {
_disposeQueue.stream.listen((obj) => obj.dispose());
},
(error, stack) {
if (error is NoSuchMethodError) {
return;
}
final errorStr = error.toString();
if (!errorStr.contains('dispose\$0 is not a function') &&
!errorStr.contains('has no instance method \'dispose\'')) {
throw error;
}
},
);
}
class Disposable {
var disposed = false;
void dispose() {
disposed = true;
}
}
class DisposeQueueDynamicDispatchBenchmark extends BenchmarkBase {
final dynamic disposable;
DisposeQueueDynamicDispatchBenchmark(this.disposable)
: super('Dynamic dispatch (inside dispose queue)');
@override
void run() {
_disposeQueue.add(disposable);
}
}
void main() {
// Pay the cost of setting up a Zone ahead of time, once
_runDisposeQueue();
// Create a disposable object
final disposable = Disposable();
// Run the benchmark
final benchmark = DisposeQueueDynamicDispatchBenchmark(disposable);
benchmark.report();
}
gives me
Dynamic dispatch (inside dispose queue)(RunTime): 0.66006125 us.
So, special casing seems to be the only option to measurably improve the performance here. I'll merge this soon. Thanks.
Since this package is most likely to be used with
ChangeNotifier
s andValueNotifier
s; it's safer and more performant to just do a type check to dispose them without resorting to dynamic dispatch.