Document flutter::RuntimeController. #17250

chinmaygarde · 2020-03-22T05:03:45Z

No description provided.

chinmaygarde · 2020-03-22T05:04:00Z

Will land on red to unblock LUCI which is red on a goma flake.

chinmaygarde · 2020-03-22T05:12:50Z

TBR

dnfield · 2020-03-22T08:35:17Z

runtime/runtime_controller.h

+/// normal operation, a single instance of this object is owned by the engine
+/// per shell. This object may only be created, used, and collected on the UI
+/// task runner. Window state queried by the root isolate is stored by this
+/// object. In cold-restart scenarios, the engine may collect this collect


cold-restart is used in other places, but I think it's a bit incongruent with our more typical terminology at this point of hot reload/hot restart.

Also - collect this collect - just collect this?

cold-restart is used in other places, but I think it's a bit incongruent with our more typical terminology at this point of hot reload/hot restart.

I didn't know we settled on that terminology already. When did we do that? I'll change all references to cold-restart in a subsequent patch.

Also - collect this collect - just collect this?

Patching.

dnfield · 2020-03-22T08:37:02Z

runtime/runtime_controller.h

+  /// @param[in]  advisory_script_uri         The advisory script URI (only used
+  ///                                         for debugging). This does not
+  ///                                         affect the code being run in the
+  ///                                         isolate in any way.
+  /// @param[in]  advisory_script_entrypoint  The advisory script entrypoint
+  ///                                         (only used for debugging). This
+  ///                                         does not affect the code being run
+  ///                                         in the isolate in any way. The
+  ///                                         isolate must be transitioned to
+  ///                                         the running state explicitly by
+  ///                                         the caller.


My understanding at this point is that these don't actually do anything at all, and don't show up anywhere meaningfully for debuggers. Is that correct?

I think these still show up when you use the debugger from the observatory. It might be a good idea to just remove these from the engine and see what breaks really.

dnfield · 2020-03-22T08:37:59Z

runtime/runtime_controller.h

+  ///                                         isolate to collect resources that
+  ///                                         may reference resources on the
+  ///                                         GPU.
+  /// @param[in]  image_decoder               The image decoder


Might be helpful to explain why this is here and not on the io_manager, or how it's different.

The IO manager does more than just aid in image decode. I think the difference will become clearer once I document that.

dnfield · 2020-03-22T08:38:09Z

runtime/runtime_controller.h

+  ///                                         code in isolate scope when the VM
+  ///                                         is about to be notified that the
+  ///                                         engine is going to be idle.
+  /// @param[in]  window_data                 The window data (if exists).


What's window data? :)

Maybe even just a link to the WindowData struct would do.

Doxygen seem to be inferring a link the struct already https://engine.chinmaygarde.com/classflutter_1_1_runtime_controller.html#af7c1055c85ff7300fdb2fd2d9576440a

dnfield · 2020-03-22T08:39:04Z

runtime/runtime_controller.h

+  /// @brief      Clone the the runtime controller. This re-creates the root
+  ///             isolate with the same snapshots and copies all window data to
+  ///             the new instance. This is usually only used in the debug
+  ///             runtime mode to support the cold-restart scenario.


Same nit about cold-restart.

dnfield · 2020-03-22T08:44:09Z

runtime/runtime_controller.h

+  /// NotifyIdle is advisory and not making the same or making it with
+  /// insufficient deadlines does not mean that the VM will keep consuming
+  /// memory indefinitely. Even if the engine made absolutely no calls to


This would be a little clearer if phrased without the double negative... something like

NotifyIdle is advisory. The VM may or may not run a garbage collection when this is called, and will eventually perform garbage collections even if it is not called or it is called with insufficient deadlines.

?

dnfield · 2020-03-22T08:45:50Z

runtime/runtime_controller.h

+  /// The garbage collection mechanism and its thresholds are internal
+  /// implementation details and absolutely no guarantees are made about the
+  /// threshold discussed below. This discussion is also an oversimplification
+  /// but hopefully serves to calibrate expectations about GC behavior:


Is there Dart documentation we could link to here instead of all of this?

People who read this will probably think it's more precise than it is, and it seems likely that this could drift out of sync with actual implementation details it's trying to capture.

Alternatively, would it make sense to move some of this to somewhere in //dart/runtime/vm where it could be maintained by the VM team? It is a really helpful description.

Is there Dart documentation we could link to here instead of all of this?

No. It's not documented anywhere. And probably for the same reasons we hesitate to document it here. It's implementation details and may change.

People who read this will probably think it's more precise than it is, and it seems likely that this could drift out of sync with actual implementation details it's trying to capture.

Fair point. But the caveats (stuff like "are implementation details and absolutely no guarantees ... oversimplification" ) I believe are sufficient for reasonable readers to not make assumptions of stability.

dnfield · 2020-03-22T08:48:00Z

runtime/runtime_controller.h

  Dart_Port GetMainPort();

+  //----------------------------------------------------------------------------
+  /// @brief      Gets the debug name of the root isolate. But default, the


This answers my question(s) above about advisory*

Maybe link to this from there?

dnfield · 2020-03-22T08:48:50Z

runtime/runtime_controller.h

+  /// @brief      Returns if the root isolate has any live receive ports.
+  ///
+  /// @return     True if there are live receive ports, False otherwise. Return
+  ///             False is the root isolate is not running as well.


Would be helpful to state why this matters here - e.g. this effectively tells you if the isolate is running or not, right?

A running isolate can have no receive ports.... right?

dnfield · 2020-03-22T08:49:39Z

runtime/runtime_controller.h

  bool HasLivePorts();

+  //----------------------------------------------------------------------------
+  /// @brief      Get the last error encountered by the microtask queue.


Might be helpful to explain that this doesn't get actual details of errors thrown in Dart code, but basically exit codes from the VM... right?

Isn't that implied from the return type of DartErrorHandleType?

Based on feedback gather in flutter#17250 after it landed.

chinmaygarde · 2020-03-22T19:53:22Z

Patches based on feedback here applied in #17256

Based on feedback gather in #17250 after it landed.

Document flutter::RuntimeController.

ed1bb3e

chinmaygarde requested review from cbracken, dnfield and jason-simmons March 22, 2020 05:03

googlebot added the cla: yes label Mar 22, 2020

chinmaygarde merged commit 31c4727 into flutter:master Mar 22, 2020

chinmaygarde deleted the document_rc branch March 22, 2020 05:12

dnfield reviewed Mar 22, 2020

View reviewed changes

chinmaygarde added a commit to chinmaygarde/flutter_engine that referenced this pull request Mar 22, 2020

Documentation cleanups to RuntimeController.

526c923

Based on feedback gather in flutter#17250 after it landed.

chinmaygarde mentioned this pull request Mar 22, 2020

Documentation cleanups to RuntimeController. #17256

Merged

dnfield pushed a commit that referenced this pull request Mar 22, 2020

Documentation cleanups to RuntimeController. (#17256)

9c64029

Based on feedback gather in #17250 after it landed.

engine-flutter-autoroll mentioned this pull request Mar 24, 2020

Roll engine 2d42c74a348d..d7ad50c96f05 (54 commits) flutter/flutter#53178

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2020

31c4727 Document flutter::RuntimeController. (flutter/engine#17250)

20b82bb

engine-flutter-autoroll mentioned this pull request Mar 24, 2020

Roll engine 2d42c74a348d..3a68f689996f (56 commits) flutter/flutter#53196

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2020

31c4727 Document flutter::RuntimeController. (flutter/engine#17250)

44832df

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2020

31c4727 Document flutter::RuntimeController. (flutter/engine#17250)

30a889f

engine-flutter-autoroll mentioned this pull request Mar 25, 2020

Roll engine 2d42c74a348d..3a68f689996f (56 commits) flutter/flutter#53210

Merged

Document flutter::RuntimeController. #17250

Document flutter::RuntimeController. #17250

Uh oh!

Conversation

chinmaygarde commented Mar 22, 2020

Uh oh!

chinmaygarde commented Mar 22, 2020

Uh oh!

chinmaygarde commented Mar 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants