-
Notifications
You must be signed in to change notification settings - Fork 19
benchmark: cronet benchmarks and comparison with dart:io #8
Conversation
benchmarks/latency.dart
Outdated
} | ||
|
||
static Future<double> measureFor(Function f, int minimumMillis) async { | ||
var minimumMicros = minimumMillis * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid writing millis and micros, you can use Duration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
Though, future delayed gives us most "time accurate" and faster results, it doesn't ensures if "late" requests are returned or not. As, we are gradually incrementing the no. of spawned tasks in each loop, we must ensure that everything is cleaned up from the previous test.
Some high level comments:
|
@dcharkes Done 😄 |
benchmarks/benchmarking.md
Outdated
2. Create a new `app.py` file that contains - | ||
|
||
```python | ||
from waitress import serve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we ideally put the code required for local servers in the repository?
cc @dcharkes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm.. I can put that if that's what best practice is. But, any server will do the job. I just chosen these 2 servers as they are easy to setup, well known and used in production.
Should I include all the codes in the repo? (And, the resources they served too?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added :)
benchmarks/run_all.dart
Outdated
void main(List<String> args) async { | ||
var url = 'https://example.com'; | ||
var throughputPrallelLimit = 10; | ||
switch (args.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also consider using package:args
throughout the package for handling cmd line args. But that's probably good for later if we end up with more arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Let's use package:args
in other places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a separate PR maybe to keep the commit history more meaningful? As, I'll have to migrate other parts of the package too. (i.e. bin/setup.dart
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! ✌️
benchmarks/latency.dart
Outdated
|
||
Future<double> report() async { | ||
final runtime = await measure(); | ||
print('Cronet(RunTime): $runtime us'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think microseconds is a bit hard to read. Let's do milliseconds.
benchmarks/run_all.dart
Outdated
final jitLatency = await CronetBenchmark.main(url); | ||
print('AOT'); | ||
Process.runSync('dart', ['compile', 'exe', 'benchmarks/latency.dart']); | ||
final aotLatency = double.parse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also stream the stdout to stdout instead of just capturing it.
benchmarks/run_all.dart
Outdated
url, pow(2, throughputPrallelLimit).toInt()); | ||
print('AOT'); | ||
Process.runSync('dart', ['compile', 'exe', 'benchmarks/throughput.dart']); | ||
final throughputStdout = Process.runSync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also stream the stdout to stdout instead of just capturing it.
benchmarks/latency.dart
Outdated
|
||
import 'package:cronet/cronet.dart'; | ||
|
||
class CronetBenchmark { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LatencyBenchmark with subclasses CronetLatencyBenchmark and DartIOLatencyBenchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference between two will be swapping the import
statement. But, how can I achieve this without 2 near duplicate file? How can I make this subclass structure?
(Maybe it's a really basic question to ask 😅 )
benchmarks/throughput.dart
Outdated
|
||
import 'package:cronet/cronet.dart'; | ||
|
||
class CronetThroughputBenchmark { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ThroughputBenchmark with subclasses CronetThroughputBenchmark and DartIOThroughputBenchmark.
benchmarks/run_all.dart
Outdated
}); | ||
|
||
print('Latency Test Results'); | ||
print('| Mode | package:cronet |'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add column dart:io
Latency Test Results
On my machine, Cronet is faster. 😄 Throughput Test Results
Can we make the time configurable for this benchmark? My internet seems too fast for Dart IO to hit it's limit. 🙈 |
@dcharkes You can pass the value of N where 2^N is the max spawn limit as n CLI argument. But, then you have to mention the target server url also as n CLI argument. |
I'll give these cli args some name on the next pr as we'll move to |
benchmarks/latency.dart
Outdated
} | ||
// TODO: https://github.com/google/cronet.dart/issues/11 | ||
await CronetLatencyBenchmark.main(url); | ||
if (benchmarkDartIO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just always run both instead of adding command line argument.
benchmarks/latency.dart
Outdated
} | ||
|
||
class DartIOLatencyBenchmark extends LatencyBenchmark { | ||
final String url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be in LatencyBenchmark?
benchmarks/run_all.dart
Outdated
break; | ||
case 2: | ||
url = args[0]; | ||
throughputPrallelLimit = int.parse(args[1]).toInt(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more common to use flags for arg parsing so that you can provide them in any order and in any combination.
e.g. If I want to just provide a different throughputPrallelLimit I don't want to type http://example.com
benchmarks/throughput.dart
Outdated
} | ||
|
||
class DartIOThroughputBenchmark extends ThroughputBenchmark { | ||
final String url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be in ThroughputBenchmark?
500 ms results: Throughput Test Results
Prints for 1 second: DartIOThroughputBenchmark: Total Spawned: 1, Returned in time: 1. |
Okay, moving to package:args in a next PR is okay. Just disregard my comments on the CLI for now then. |
@dcharkes Opps! Seems like I misunderstood your comment. Anyways, Duration is now configurable as a 3rd argument from CLI. Other changes are done except the CLI ones :). Added MacOS as supported platform in README. Seems like we missed that in the other PR. |
🚀 🚀 🚀 |
Benchmarks
Package Version:
0.0.1+1
Cronet Version:
86.0.4240.198
Cronet Build Type:
Debug
Wrapper Version:
1
Dart SDK Version:
2.12.0
dart:io
dart:io
May get affected by #11
Closes #3