[fuchsia] Use FFI to get System clockMonotonic #27353

iskakaushik · 2021-07-13T13:38:09Z

This is a part of a larger effort to remove the
dependency on tonic and have the Fuchsia runner live
independently

flutter-dashboard · 2021-07-13T13:38:11Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

iskakaushik · 2021-07-13T13:45:49Z

shell/platform/fuchsia/dart-pkg/zircon_ffi/pubspec.yaml

+
+dependencies:
+  ffi: ^1.0.0
+  path: ^1.8.0


this should be removed.

chinmaygarde · 2021-07-13T14:02:52Z

shell/platform/fuchsia/dart-pkg/zircon_ffi/lib/zircon_ffi.dart

+  late final _zircon_dart_clock_get_monotonic_ptr =
+      _lookup<ffi.NativeFunction<_c_zircon_dart_clock_get_monotonic>>(
+          'zircon_dart_clock_get_monotonic');
+  late final _dart_zircon_dart_clock_get_monotonic


Why not call zx_clock_get_monotonic directly from DynamicLibrary.process? Creating a separate library with a trampoline doesn't seem to be adding any value. Perhaps you wanted to build on top of this?

@chinmaygarde, yup, I'm planning on building on top of this. Choosing a simple method like clock monotonic was the easiest way for me to test the combination of:

ffigen code generation within the engine repo.

Integration of dart:zircon_ffi with dart:zircon.

Testing this whole bundle in the fuchsia tree. (We do not test this code in the engine repo).

After this change, I wish to move handle_waiter.dart and handle.dart to the FFI model (away from tonic) as well.

I feel like chinmay's question still has merit here. We will need a more complex trampoline as time goes on for sure, but why put the ZirconFFIBindings code into a separate .so, instead of linking in into the runner binary and using DynamicLibrary.process? I'm probably missing something, but I can only see an upside to that decision because its less artifacts to distribute

You run into symbol visibility issues and the linker or LTO thinking it can get rid of symbols with default visibility because they are not used. Kaushik and I were experimenting with it this morning and having a tough time making resolving symbols that way.

I know this is already merged but I have experimented with this before and didn't need to do any extra trampolining by adding this method to the package:zircon instead of dart:zircon in fuchsia.git. The zx_* functions in libzircon get linked in at runtime by the os so you can get at this function by just using DynamicLibrary.process.

chinmaygarde

Minor suggestion about explicitly setting the symbol visibility to default. Not sure why its not hidden by default actually. Other than that, lgtm.

chinmaygarde · 2021-07-13T14:10:46Z

shell/platform/fuchsia/dart-pkg/zircon/lib/src/init.dart

+    if (_bindings == null) {
+      final _dylib = DynamicLibrary.open(_kLibZirconDartPath);
+      _bindings = ZirconFFIBindings(_dylib);
+      // final initializer = _bindings?.zircon_dart_dl_initialize;


I don't think you meant to leave this commented code in.

chinmaygarde · 2021-07-13T14:11:34Z

shell/platform/fuchsia/dart-pkg/zircon_ffi/clock.h

+extern "C" {
+#endif
+
+uint64_t zircon_dart_clock_get_monotonic();


Perhaps flip the symbol visibility explicitly to default.

chandarrengoog · 2021-07-13T16:31:12Z

shell/platform/fuchsia/dart-pkg/zircon/lib/src/system.dart

@@ -236,7 +236,9 @@ class System extends NativeFieldWrapperClass2 {
  static MapResult vmoMap(Handle vmo) native 'System_VmoMap';

  // Time operations.
-  static int clockGetMonotonic() native 'System_ClockGetMonotonic';
+  static int clockGetMonotonic() {


btw... I don't think we currently have dart unittest/integration test coverage for this method: https://cs.opensource.google/search?q=%22clockGetMonotonic%22&sq=&ss=fuchsia%2Ffuchsia:sdk%2Fdart%2Fzircon%2Ftest%2F

Just wanted to make sure if you've manually checked if this worked since our existing tests don't cover this. 🙂

I noticed it, I was testing using https://fuchsia-review.googlesource.com/c/fuchsia/+/554605

chandarrengoog · 2021-07-13T16:32:51Z

shell/platform/fuchsia/dart-pkg/zircon_ffi/pubspec.yaml

+  path: ^1.8.0
+
+dev_dependencies:
+  ffigen: ^3.0.0


nice! I didn't know this existed. 😀

arbreng · 2021-07-13T16:38:26Z

shell/platform/fuchsia/dart-pkg/zircon/lib/src/init.dart

+
+final _kLibZirconDartPath = '/pkg/lib/libzircon_ffi.so';
+
+class _Bindings {


Why not put this logic directly into ZirconFFIBindings and expose an instance getter, then have systen.dart call ZirconFFIBindings.instance? This is how the framework does things and its nice

ZirconFFIBindings is autogenerated using ffigen and I wanted to keep it that way.

arbreng · 2021-07-13T16:42:27Z

shell/platform/fuchsia/dart-pkg/zircon_ffi/lib/zircon_ffi.dart

+  late final _zircon_dart_clock_get_monotonic_ptr =
+      _lookup<ffi.NativeFunction<_c_zircon_dart_clock_get_monotonic>>(
+          'zircon_dart_clock_get_monotonic');
+  late final _dart_zircon_dart_clock_get_monotonic


I feel like chinmay's question still has merit here. We will need a more complex trampoline as time goes on for sure, but why put the ZirconFFIBindings code into a separate .so, instead of linking in into the runner binary and using DynamicLibrary.process? I'm probably missing something, but I can only see an upside to that decision because its less artifacts to distribute

This is a part of a larger effort to remove the dependency on tonic and have the Fuchsia runner live independently

…e#27353)

* a7b5522 refactor and simplify CI dart analysis (flutter/engine#27370) * 137009b Switch test_suites to yaml. (flutter/engine#27368) * a22a3ca [web] fix a few analysis lints (flutter/engine#27375) * a02c017 make it work on <API 24 (flutter/engine#27398) * 0220256 Make FlutterFragment usable without requiring it to be attached to an Android Activity. (Attempt 2) (flutter/engine#27397) * 51e07a5 [fuchsia] Use FFI to get System clockMonotonic (flutter/engine#27353) * 4015d8b Roll Skia from 224e3e257d06 to 773a0b8c7e74 (44 revisions) (flutter/engine#27399) * 7db1a96 [ci.yaml] Add Linux Android Scenarios postsubmit (flutter/engine#27400) * a02f6bc remove the use of package:isolate (flutter/engine#27401) * 3237f4f Roll Skia from 773a0b8c7e74 to 36c1804e8f5c (1 revision) (flutter/engine#27402) * 75af7c8 Roll Dart SDK from ab009483f343 to 746879714c96 (5 revisions) (flutter/engine#27403) * be21e40 [ci.yaml] Add linux benchmarks, add enabled branches (flutter/engine#27405) * c8d7a97 Roll Fuchsia Mac SDK from wUg-tGGCL... to uhahzGJ6H... (flutter/engine#27408) * 3649200 Roll Skia from 36c1804e8f5c to 947a2eb3c043 (7 revisions) (flutter/engine#27410) * f04d941 [web] enable always_specify_types lint (flutter/engine#27406) * bdaaa4f [fuchsia] fix race in DefaultSessionConnection (flutter/engine#27377) * 84247f2 Update the Fuchsia runner to use fpromise instead of fit::promise (flutter/engine#27416) * 39119d2 Roll Skia from 947a2eb3c043 to 9081276b2907 (6 revisions) (flutter/engine#27426) * 9002bc7 Roll Skia from 9081276b2907 to 0547b914f691 (2 revisions) (flutter/engine#27430) * 58e0688 Roll Fuchsia Linux SDK from hykYtaK7D... to s2vrjrfuS... (flutter/engine#27431) * 1dca887 Roll Dart SDK from 746879714c96 to d53eb1066384 (2 revisions) (flutter/engine#27432) * c9008f3 Use python to run firebase testlab, do not expect recipe to know location of APK (flutter/engine#27434) * 8f7c529 Roll Skia from 0547b914f691 to 7d336c9557bd (3 revisions) (flutter/engine#27436) * 534404e Roll Fuchsia Mac SDK from uhahzGJ6H... to TWPguQ-ow... (flutter/engine#27438) * 9f13308 Roll Dart SDK from d53eb1066384 to fcbaa0a90b4b (1 revision) (flutter/engine#27439) * e5e7b94 Roll Skia from 7d336c9557bd to 7dc26fadc90b (2 revisions) (flutter/engine#27440) * bf3d265 Roll Skia from 7dc26fadc90b to dd561d021470 (1 revision) (flutter/engine#27442) * 6e62915 [ci.yaml] Add xcode property to ci.yaml (flutter/engine#27415) * 33c17a1 Roll Skia from dd561d021470 to 0e99fbe5da5c (1 revision) (flutter/engine#27443) * 0bc2479 Adjust web_sdk rule deps (flutter/engine#27435) * 7a8969a [web] enable prefer_final_locals lint (flutter/engine#27420) * 590902b Roll Dart SDK from fcbaa0a90b4b to 207232b5abe0 (1 revision) (flutter/engine#27446) * 283a42f fuchsia: Log vsync stats in inspect (flutter/engine#27433) * 4af14b9 Deeplink URI fragment on Android and iOS (flutter/engine#26185) * 47899db Remove unused generate_dart_ui target (flutter/engine#27445) * fb265c2 Roll Skia from 0e99fbe5da5c to a2d22b2e085e (3 revisions) (flutter/engine#27447) * 8bb5760 [ci.yaml] Mark Linux Android Scenarios as flaky (flutter/engine#27422)

iskakaushik requested review from arbreng and chandarrengoog July 13, 2021 13:38

flutter-dashboard bot added the platform-fuchsia label Jul 13, 2021

flutter-dashboard bot added the needs tests label Jul 13, 2021

google-cla bot added the cla: yes label Jul 13, 2021

iskakaushik commented Jul 13, 2021

View reviewed changes

chinmaygarde reviewed Jul 13, 2021

View reviewed changes

chinmaygarde approved these changes Jul 13, 2021

View reviewed changes

chandarrengoog approved these changes Jul 13, 2021

View reviewed changes

chandarrengoog reviewed Jul 13, 2021

View reviewed changes

arbreng approved these changes Jul 13, 2021

View reviewed changes

iskakaushik added 4 commits July 14, 2021 16:54

[fuchsia] Use FFI to get System clockMonotonic

893fd97

This is a part of a larger effort to remove the dependency on tonic and have the Fuchsia runner live independently

soft transition

f983546

fix licenses

b8061c9

archive contains libzircon_ffi.so

c7762e9

iskakaushik force-pushed the fuchsia_clock_monotonic_ffi branch from e2e354d to c7762e9 Compare July 14, 2021 17:19

remove unused path dep

0badbf1

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 14, 2021

iskakaushik merged commit 51e07a5 into flutter:master Jul 14, 2021

iskakaushik deleted the fuchsia_clock_monotonic_ffi branch July 14, 2021 18:02

engine-flutter-autoroll mentioned this pull request Jul 15, 2021

Roll Engine from 093c4ed1a3d7 to 6491d17331ab (39 revisions) flutter/flutter#86505

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 15, 2021

51e07a5 [fuchsia] Use FFI to get System clockMonotonic (flutter/engin…

17075d8

…e#27353)

engine-flutter-autoroll mentioned this pull request Jul 15, 2021

Roll Engine from 093c4ed1a3d7 to 8bb5760c0920 (37 revisions) flutter/flutter#86508

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 15, 2021

51e07a5 [fuchsia] Use FFI to get System clockMonotonic (flutter/engin…

31110ec

…e#27353)

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

[fuchsia] Use FFI to get System clockMonotonic (flutter#27353)

d5c05bf

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

[fuchsia] Use FFI to get System clockMonotonic (flutter#27353)

81621d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fuchsia] Use FFI to get System clockMonotonic #27353

[fuchsia] Use FFI to get System clockMonotonic #27353

iskakaushik commented Jul 13, 2021

flutter-dashboard bot commented Jul 13, 2021

iskakaushik Jul 13, 2021

chinmaygarde Jul 13, 2021

iskakaushik Jul 13, 2021

arbreng Jul 13, 2021

chinmaygarde Jul 13, 2021

chaselatta Jul 16, 2021

chinmaygarde left a comment

chinmaygarde Jul 13, 2021

chinmaygarde Jul 13, 2021

chandarrengoog Jul 13, 2021

iskakaushik Jul 13, 2021

chandarrengoog Jul 13, 2021

arbreng Jul 13, 2021

iskakaushik Jul 13, 2021

arbreng Jul 13, 2021


		final _kLibZirconDartPath = '/pkg/lib/libzircon_ffi.so';

		class _Bindings {

[fuchsia] Use FFI to get System clockMonotonic #27353

[fuchsia] Use FFI to get System clockMonotonic #27353

Conversation

iskakaushik commented Jul 13, 2021

flutter-dashboard bot commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment