[Impeller] add a configuration option that allows defering all PSO construction until needed. #165261
Conversation
… into defer_construction
| struct Settings { | ||
| bool lazy_shader_mode = false; | ||
| }; |
There was a problem hiding this comment.
I'm working on a flags system for impeller right now. I think we're better off with a holistic flags system for all of impeller instead of these different settings distributed across the codebase and sometimes living as parameter arguments.
I planned on migrating the wifeframe option to this system too.
It will be something like this:
namespace impeller {
class Flags {
public:
void Init(std::set<std::string_view> names);
bool HasFlag(std::string_view name) const;
};
}
There was a problem hiding this comment.
slightly related, I'd actually like us to remove wireframe as its a debugging option we no longer use.
better flags SGTM tho. Barring other feedback, I'd like to land this first to unblock the customer money tests. If there are things I can change to make it work better with your flag proposal that would be fine too.
There was a problem hiding this comment.
Okay, here's the changes I propose:
- Get rid of
ContentContext::Settings - Get rid of
AiksContext::Settings - Add a new class, impeller::Flags that has one flag
enable_lazy_shader_mode. - Make impeller::Context have an impeller::Flags, that way people can get flags from the Context directly, or from
ContentContext->GetContext() - Any place that you are passing an argument that is like
bool lazyinstead pass a const reference to impeller::Flags.
This has the benefits of:
- One singular place for added flags, it's easier to add more flags in the future
Settingsisn't overloaded- Signatures like the
ContentContextconstructor won't change
Lay it out however is easiest for you, I can tweak it later if need be. I can also do the patch for you in this pr, or author a different pr you can rebase on if you want.
There was a problem hiding this comment.
Okay, I refactored it to use impeller::Flags. I also uncommented out that function call like we discussed in the other PR. Let's try getting this through together, if it gets too weird I can rip out the flags stuff into a new pr and we can rebase this pr on it.
|
autosubmit label was removed for flutter/flutter/165261, because - The status or check suite Linux linux_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/165261, because - The status or check suite CI Caught Failure has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/165261, because - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
reason for revert: bork the tree |
…ll PSO construction until needed. (#165261)" (#165619) <!-- start_original_pr_link --> Reverts: #165261 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: jonahwilliams <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: bork the tree <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: jonahwilliams <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {gaaclarke} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: The cost of bootstapping the initial PSOs can regress cold startup time for customer money. As an experiment, attempt to defer PSO construction to skia like. <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <flutter-engprod-team@google.com>
Aw man, I needed the flags work for antialias lines. Should I split that into a different PR or do you see us relanding this today? |
|
We can reland the bork was unrelated |
The cost of bootstapping the initial PSOs can regress cold startup time for customer money. As an experiment, attempt to defer PSO construction to skia like.