Skip to content
This repository was archived by the owner on Feb 11, 2024. It is now read-only.

move to package:args for cli arg parsing #15

Merged
merged 6 commits into from
Jul 16, 2021

Conversation

unsuitable001
Copy link
Contributor

@unsuitable001 unsuitable001 commented Jul 15, 2021

Continuation #8

  • Bumped up the version of every dependency package as much as possible.
  • Use package:args for CLI argument parsing.
      • bin/setup.dart
      • tool/ files. (Tools don't take any CLI arg)
      • benchmark/ files.
  • Fixes wrong location issue in run_all.dart due to the migration from benchmarks/ to benchmark/ (rename benchmark and example folder #14 )
  • Fixed wrong cli suggestion in case of un-locateable dylib.
  • Fixes RangeError (index) in benchmark/throughput.dart in case of 0 result.
  • Added README.md to example folder to fetch more pub points.

Closes #12

@unsuitable001
Copy link
Contributor Author

unsuitable001 commented Jul 15, 2021

@dcharkes Remind me to add the changelog and do the version bump after review (if required).

Otherwise, done!

@dcharkes
Copy link
Member

Yeah, we want a changelog entry and version bump to 0.0.3 because we're changing the CLI, which is a breaking change.

@dcharkes
Copy link
Member

@mannprerak2, I'll leave the reviewing for this one up to you. 🚀

@unsuitable001
Copy link
Contributor Author

Bumped the version, added the changelog and few minor touches.

Copy link
Contributor

@mannprerak2 mannprerak2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻

final aotLantencyProc = await Process.start('benchmarks/latency.exe', [url]);
Process.runSync('dart', ['compile', 'exe', 'benchmark/latency.dart']);
final aotLantencyProc =
await Process.start('benchmark/latency.exe', ['-u', url]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the 'exe' files compiled by dart only work on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No 😅. It is named as .exe by default regardless of the platform.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. I see it now in the documentation. Dart supports quite a few different compilation formats 😀

}
final url = arguments['url'] as String;
final throughputPrallelLimit = int.parse(arguments['limit'] as String);
final duration = Duration(seconds: int.parse(arguments['time'] as String));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work with half a second?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Duration's seconds param takes int.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should do milliseconds instead then, or convert a double manually. Because otherwise we can't test shorter durations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with milliseconds then 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got this result under short duration. Cronet is so much faster.

Latency Test Results

Mode package:cronet dart:io
JIT 247.333 ms 403.000 ms
AOT 240.444 ms 373.000 ms

Throughput Test Results (Duration: 500 ms)

Mode package:cronet dart:io
JIT 30 out of 1024 0 out of 0
AOT 26 out of 1024 0 out of 0

@unsuitable001
Copy link
Contributor Author

@dcharkes Reverted. Duration is now in second(s) again :)

@dcharkes dcharkes merged commit 80df427 into google:main Jul 16, 2021
@unsuitable001 unsuitable001 deleted the package_args branch July 16, 2021 14:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use package:args
3 participants