Repository navigation
[Impeller] Fix the text benchmark on Impeller backends - #192677
Conversation
The DrawTextBlob benchmark always built a DlTextSkia, whose text frame is null for Impeller backends, crashing the dispatcher. Build a DlTextImpeller when the surface provider targets Impeller. Benchmark-only change; no production code is modified. test-exempt: benchmark-only crash fix.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the display list benchmarks to support Impeller rendering under the IMPELLER_SUPPORTS_RENDERING flag, conditionally drawing text blobs using DlTextImpeller when targeting Impeller. Feedback suggests optimizing performance within the benchmark loop by using std::move(blob) to avoid unnecessary atomic reference count overhead when creating the text blobs.
| #ifdef IMPELLER_SUPPORTS_RENDERING | ||
| if (targets_impeller) { | ||
| builder.DrawText(DlTextImpeller::MakeFromBlob(blob), 50.0f, 50.0f, paint); | ||
| continue; | ||
| } | ||
| #endif // IMPELLER_SUPPORTS_RENDERING | ||
| builder.DrawText(DlTextSkia::Make(blob), 50.0f, 50.0f, paint); |
There was a problem hiding this comment.
Since blob is a sk_sp<SkTextBlob> and is not used after being passed to DrawText, we can use std::move(blob) to avoid unnecessary atomic reference count increments and decrements. This is especially beneficial in a benchmark context to minimize overhead.
#ifdef IMPELLER_SUPPORTS_RENDERING
if (targets_impeller) {
builder.DrawText(DlTextImpeller::MakeFromBlob(std::move(blob)), 50.0f, 50.0f, paint);
continue;
}
#endif // IMPELLER_SUPPORTS_RENDERING
builder.DrawText(DlTextSkia::Make(std::move(blob)), 50.0f, 50.0f, paint|There was a problem hiding this comment.
MakeFromBlob and DlTextSkia::Make both take const sk_sp&, so std::move at this call site does not transfer ownership. MakeFromBlob does not retain the blob, and DlTextSkia still copies into blob_. This construction is also outside the timed RenderDisplayList loop, so it would not affect the measured numbers.
There was a problem hiding this comment.
Code Review
This pull request introduces Impeller-specific rendering logic to the display list benchmarks by adding conditional compilation guards and updating the benchmark loop to utilize DlTextImpeller when the surface provider targets Impeller. A review comment was provided suggesting a null check for the surface_provider before invoking its methods to prevent potential null pointer dereferences.
| #ifdef IMPELLER_SUPPORTS_RENDERING | ||
| const bool targets_impeller = surface_provider->TargetsImpeller(); | ||
| #endif // IMPELLER_SUPPORTS_RENDERING |
There was a problem hiding this comment.
To prevent a potential null pointer dereference, it is safer to check if surface_provider is non-null before calling TargetsImpeller() on it.
| #ifdef IMPELLER_SUPPORTS_RENDERING | |
| const bool targets_impeller = surface_provider->TargetsImpeller(); | |
| #endif // IMPELLER_SUPPORTS_RENDERING | |
| #ifdef IMPELLER_SUPPORTS_RENDERING | |
| const bool targets_impeller = surface_provider && surface_provider->TargetsImpeller(); | |
| #endif // IMPELLER_SUPPORTS_RENDERING |
There was a problem hiding this comment.
No change needed. Create() returns nullptr only when the backend is disabled at build time, and the same defines gate benchmark registration, so surface_provider is always non-null here. It is also dereferenced unconditionally a few lines above (InitializeSurface), like every other benchmark in this file.
There was a problem hiding this comment.
Passed on my local mac. LGTM with one nit: Please update the PR description where primitive_rendering_benchmarks is referenced. BM_DrawTextBlob is compiled under display_list_benchmarks via DISPLAY_LIST_BENCHMARK_ALL_OPS, whereas primitive_rendering_benchmarks omits text benchmarks.
|
Updated the description. Thanks for the review! |
andywolff
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/192677, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
gaaclarke
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
The DrawTextBlob benchmark always built a DlTextSkia, whose text frame is null on Impeller backends. The dispatcher FML_CHECKs that frame and aborts.
This is a local macOS/iOS benchmark crash. The text benchmark is compiled into display_list_benchmarks via DISPLAY_LIST_BENCHMARK_ALL_OPS (primitive_rendering_benchmarks omits text benchmarks); the Impeller Metal variants are built only when ENABLE_METAL_BENCHMARKS is set. Linux CI does not run those backends.
Build a DlTextImpeller when the surface provider targets Impeller. The backend check is outside the timed loop.
Fixes #192671
test-exempt: benchmark-only crash fix
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Made with Cursor