Add pipeline_state_observer interface #8970

nathanrogersgoogle · 2019-05-15T06:33:04Z

Add a new pipeline observer interface, specified via an optionally set
|std::function| on |flutter::Settings|. The callback is called by
|Pipeline|, and provides information similar to what is implied by the
existing pipeline flow events. The difference between the flow events
and the new interface, is that this information is available at runtime
even when tracing isn't enabled, and can be sent to a region of code
that is able to call platform specific APIs. For example, one might
want to provide this information to a system metrics API.

PT-154
CF-776

Add a new pipeline observer interface, specified via an optionally set |std::function| on |flutter::Settings|. The callback is called by |Pipeline|, and provides information similar to what is implied by the existing pipeline flow events. The difference between the flow events and the new interface, is that this information is available at runtime even when tracing isn't enabled, and can be sent to a region of code that is able to call platform specific APIs. For example, one might want to provide this information to a system metrics API. PT-154 CF-776

chinmaygarde

The overall direction seems sound. There are just a few nits and the issue with the abort not being fired. I dont see a unit-test for the pipeline. I'd appreciate if you added one to the "shell_unittests" target. But, if you dont want to set that up, file and issue for that and I'll wire it up. Thanks.

chinmaygarde · 2019-05-15T06:52:15Z

shell/common/animator.cc

 #include "flutter/shell/common/animator.h"

 #include "flutter/fml/trace_event.h"
+#include "flutter/shell/common/pipeline.h"


How are you able to include this file here? This is in the common library which the shell depends on. This would make it a circular dependency. Did you run GN with the --check option?

chinmaygarde · 2019-05-15T06:55:49Z

shell/common/pipeline.h


+enum class PipelineItemStateChange {
+  // Indicates that a new pipeline item has started.
+  kBegin,


Suggestion, maybe call this kProducerBegin, kProducerEnd, kConsumerBegin, kConsumerEnd/Abort. Since this is a single producer single consumer queue, I believe that would make the enum values clearer. As you noted, kStep should not happen more than once. So let's just make the names clearer.

Also, I think this Pipeline is not UI thread or GPU thread specific, as it can be used in other threads/TaskRunners, right? If so, maybe only mention UI and GPU thread as an example, and explain the high-level concept only using producer/consumer.

chinmaygarde · 2019-05-15T06:57:05Z

shell/common/pipeline.h

+// Called when pipeline items change states.  See
+// |Settings::pipeline_state_observer| for further details.
+using PipelineStateObserver =
+    std::function<void(intptr_t pipeline_id,


A shell is guaranteed to have just one frame pipeline. Specifying the ID here is redundant as that information can just be associated in the std::function captures in the embedder.

chinmaygarde · 2019-05-15T07:01:18Z

shell/common/pipeline.h

    // Ensure the queue mutex is not held as that would be a pessimization.
    available_.Signal();
+
+    if (pipeline_state_observer_) {


I think the resource is always valid. You can probably just assert that here. I case of an aborted item, I believe that item is just dropped on the floor. So I don't actually think you will ever get an abort.

If you add these state observer fire events right next to the calls to TRACE_EVENT_ASYNC BEGIN, STEP and END, you should get the right behavior.

chinmaygarde · 2019-05-15T07:08:10Z

Hmm, the presubmits caught the circular include. Maybe you are running this in the Fuchsia buildroot. I would have expected GN checks to be turned on there as well. In any case, if you move the typedef to the common header, it should fix the issue.

chinmaygarde · 2019-05-15T22:50:51Z

cc @liyuqian

liyuqian

I think this could be useful to monitor the pipeline items in general.

FYI, I'm also working on a frame build/raster specific API so the Flutter app can get those metrics in Dart: #8983

liyuqian · 2019-05-16T05:53:50Z

shell/common/pipeline.h


+enum class PipelineItemStateChange {
+  // Indicates that a new pipeline item has started.
+  kBegin,


Also, I think this Pipeline is not UI thread or GPU thread specific, as it can be used in other threads/TaskRunners, right? If so, maybe only mention UI and GPU thread as an example, and explain the high-level concept only using producer/consumer.

cbracken · 2019-05-20T17:54:58Z

@nathanrogersgoogle checking as part of weekly PR review -- do you plan to address @chinmaygarde and @liyuqian 's comments and proceed with this PR?

liyuqian · 2019-05-20T17:59:33Z

@nathanrogersgoogle and I have discussed this with my PR #8983. We concluded that #8983 could help in this use case so I'll first modify my #8983 to add the API that this needs. Nathan could probably modify this later based on #8983. I should have the updated #8983 ready today.

cbracken · 2019-06-17T17:46:17Z

@nathanrogersgoogle looks like #8983 is merged. Do you intend to proceed with this PR?

nathanrogersgoogle · 2019-06-17T22:24:02Z

Let me review the final version of #8983 and see if any further changes are required for my use case. Will try and get back to this by tomorrow or Wednesday.

nathanrogersgoogle · 2019-06-25T10:37:44Z

Alright, I had a chance to look over #8983. I think in principle it has the information I'm looking for, however in practice even after setting |flutter::Settings::frame_rasterized_callback|,

engine/shell/common/rasterizer.h

Lines 30 to 33 in ae8e6d9

    
           // TODO(dnfield): remove once embedders have caught up. 
        
           class DummyDelegate : public Delegate { 
        
             void OnFrameRasterized(const FrameTiming&) override {} 
        
           };

is what ends up getting called (and the callback I locally set at https://fuchsia.googlesource.com/topaz/+/4586d9f55297c881557474da55b49080cab1c1b3/runtime/flutter_runner/component.cc#263 does not get called). I'll sync with Yuqian on what having the callback get called on Fuchsia looks like.

If that ends up requiring a Flutter engine patch, would you prefer me to upload that as a new patch set on this CL, or to start a new CL? I'm happy to do either.

liyuqian · 2019-06-25T19:51:35Z

@nathanrogersgoogle : I think #9484 would solve your delegate problem

nathanrogersgoogle · 2019-06-29T06:11:11Z

@liyuqian #9484 indeed fixes the issue. Thanks!

Given that it looks like we can safely abandon this CL. Thanks everyone.

nathanrogersgoogle requested a review from chinmaygarde May 15, 2019 06:33

googlebot added the cla: yes label May 15, 2019

chinmaygarde suggested changes May 15, 2019

View reviewed changes

liyuqian reviewed May 16, 2019

View reviewed changes

liyuqian mentioned this pull request May 17, 2019

Add onReportTimings and FrameRasterizedCallback API #8983

Merged

cbracken added the waiting for customer response label May 20, 2019

cbracken removed the waiting for customer response label Jun 1, 2019

nathanrogersgoogle mentioned this pull request Jun 29, 2019

Set shell as Rasterizer::Delegate in Fuchsia #9484

Closed

nathanrogersgoogle closed this Jun 29, 2019

Add pipeline_state_observer interface #8970

Add pipeline_state_observer interface #8970

Uh oh!

Conversation

nathanrogersgoogle commented May 15, 2019

Uh oh!

chinmaygarde left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 15, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 15, 2019

Choose a reason for hiding this comment

Uh oh!

liyuqian May 16, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 15, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 15, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 15, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented May 15, 2019

Uh oh!

chinmaygarde commented May 15, 2019

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian May 16, 2019

Choose a reason for hiding this comment

Uh oh!

cbracken commented May 20, 2019

Uh oh!

liyuqian commented May 20, 2019

Uh oh!

cbracken commented Jun 17, 2019

Uh oh!

nathanrogersgoogle commented Jun 17, 2019

Uh oh!

nathanrogersgoogle commented Jun 25, 2019

Uh oh!

liyuqian commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathanrogersgoogle commented Jun 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chinmaygarde left a comment •

edited

Loading

liyuqian commented Jun 25, 2019 •

edited

Loading