Skip to content

Impeller: avoid blocking on image upload scheduling - #181601

Closed
FelixMittermeier wants to merge 1 commit into
flutter:masterfrom
FelixMittermeier:impeller-no-block-upload-schedule
Closed

FelixMittermeier wants to merge 1 commit into
flutter:masterfrom
FelixMittermeier:impeller-no-block-upload-schedule

Conversation

@FelixMittermeier

Copy link
Copy Markdown
Contributor

Change:

  • In ImageDecoderImpeller::UnsafeUploadTextureToPrivate, submit the image upload command buffer with block_on_schedule=false, while keeping the existing fence-based wait when available.

Why:

  • Upload scheduling was synchronously blocking the CPU, inflating UploadTextureToPrivate time and causing jank. The fence already provides correctness for GPU visibility, so we can avoid the schedule wait without changing the upload work itself.

Performance (large_image_changer macrobenchmark, iPhone 14 Pro Max, iOS 26.2, profile mode, Impeller):

  • Before: avg build 12.86ms, avg raster 27.73ms, worst raster 397.16ms; UploadTextureToPrivate max 2,731,526µs.
  • After: avg build 0.742ms, avg raster 13.07ms, worst raster 177.14ms; UploadTextureToPrivate max 6,421µs.
  • Relative: ~17.3x faster avg build, ~2.1x faster avg raster, ~425x lower UploadTextureToPrivate max.

Benchmark command:

  • flutter drive --profile -d -t test_driver/large_image_changer.dart --driver test_driver/large_image_changer_test.dart

@flutter-dashboard

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@google-cla

google-cla Bot commented Jan 28, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions Bot added the engine flutter/engine related. See also e: labels. label Jan 28, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to improve performance by making image upload scheduling non-blocking. It achieves this by setting block_on_schedule=false when submitting the command buffer. My review identifies a potential issue where a redundant blocking call might still exist, which could negate the intended performance improvement.

}
},
/*block_on_schedule=*/true)
/*block_on_schedule=*/false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Setting block_on_schedule to false correctly avoids blocking within the Submit call. However, a blocking wait appears to be re-introduced on line 553 with command_buffer->WaitUntilScheduled().

The PR description states, 'The fence already provides correctness for GPU visibility, so we can avoid the schedule wait.' Following this logic, if context->AddTrackingFence(result_texture) returns true, the subsequent call to WaitUntilScheduled() seems redundant and should likely be removed to fully realize the intended performance improvement.

@gaaclarke

Copy link
Copy Markdown
Member

Note: Jonah says this risks reintroducing the magenta error image problem. I haven't looked but I wanted to capture his esteemed opinion.

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no test for this also the author of this code says that this will increase the risk that we have errors when returning from backgrounding. I'm inclined not to make this change without significant test coverage.

@Piinks

Piinks commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

Since we have not heard back here, we are going to close this PR for now. Thank you for contributing, please feel free to re-open if you return to this work in the future.

@Piinks Piinks closed this Apr 6, 2026
pull Bot pushed a commit to TheRakeshPurohit/flutter that referenced this pull request Aug 12, 2026
…#190445)

Fixes [flutter#170599](flutter#170599).

Metal image uploads currently wait synchronously for every upload
command buffer to reach the scheduled state. That wait was added to
protect an iOS backgrounding race, but paying it after every foreground
upload unnecessarily blocks image decoding and can delay images becoming
ready.

This PR moves that synchronization to the lifecycle boundary where it is
required:

- foreground image uploads commit their Metal command buffer, register a
scheduled-or-terminal receipt, and return without waiting;
- when GPU access changes to disabled, `ContextMTL` synchronously drains
any still-pending image-upload receipts before the transition returns;
- receipt registration remains inside the enabled side of the GPU
`SyncSwitch`, so a concurrent disable transition cannot miss an upload
that was just committed;
- existing GLES tracking-fence and `glFlush`/`glFinish` behavior is
unchanged.

This is a deeper, correctness-preserving follow-up to my deliberately
oversimplified first experiment,
[flutter#181601](flutter#181601). That PR
demonstrated the performance opportunity by only changing
`block_on_schedule` to `false`, but it did not replace the iOS lifecycle
barrier and did not include the required tests. This PR implements the
receipt-based design proposed during the original review and verifies it
with deterministic concurrency tests and repeated physical-device
measurements.

Related lifecycle work includes the original magenta-image report
[flutter#166668](flutter#166668), the
scheduling wait in
[flutter#169378](flutter#169378), the
deferred-task race fix in
[flutter#169596](flutter#169596), their stable
cherry-pick [flutter#170846](flutter#170846),
and the separate initial GPU-state fix
[flutter#171785](flutter#171785). Older
Impeller synchronization discussions
[flutter#120399](flutter#120399),
[flutter#120406](flutter#120406), and
[flutter#131490](flutter#131490) are
background context only and are not closed by this PR.

## Why was every upload waiting?

The original report,
[flutter#166668](flutter#166668), described
images becoming pink/magenta after an iOS application was awakened in
the background and later foregrounded.

[flutter#169378](flutter#169378) addressed a
narrow commit-versus-background transition race:

1. A notification can wake an application while it is still in the
background.
2. Image decoding starts while the engine still considers GPU access
available.
3. The decoder commits a Metal upload command buffer.
4. The application can re-enter the background before Metal acknowledges
the command buffer as scheduled.
5. The upload can then fail under iOS background GPU restrictions,
leaving a persistent magenta image.

The fix submitted image uploads with `block_on_schedule=true`. Because
the upload runs inside the shared side of the GPU `SyncSwitch`, waiting
before that handler returns prevents the switch's exclusive disable
transition from passing until the upload is scheduled.

That is correct, but expensive: every normal upload performs the native
`[MTLCommandBuffer waitUntilScheduled]`, even when no lifecycle
transition is happening.

This cost was called out in the original review. In particular:

- [the review proposed retaining a submission receipt and waiting only
when GPU access is
lost](flutter#169378 (comment));
- [a follow-up clarified that ordinary asynchronous scheduling should
only block during
`applicationDidEnterBackground:`](flutter#169378 (comment));
- [the deadlock discussion warned against waiting after GPU access is
already
unavailable](flutter#169378 (comment)).

The tech-debt issue
[flutter#170599](flutter#170599) was then
filed specifically to remove this extra foreground thread blocking. This
PR implements that deferred design rather than removing synchronization.

## Relationship to the previous attempt

[flutter#181601](flutter#181601) only disabled
the per-upload wait. It found a very large reduction in the pathological
`UploadTextureToPrivate` span, proving that the effective Metal wait was
in the submission path. However, the review correctly identified two
blockers:

- [removing the wait risked reintroducing the magenta-image lifecycle
race](flutter#181601 (comment));
- [the change needed significant deterministic test
coverage](flutter#181601 (review)).

It was [closed because there was no follow-up at that
time](flutter#181601 (comment)).

[An automated review also suggested removing a later generic
`command_buffer->WaitUntilScheduled()`
call](flutter#181601 (comment)).
That suggestion does not account for backend behavior: Metal's generic
`WaitUntilScheduled()` and `WaitUntilCompleted()` hooks are no-ops in
this path, while the same block has real `glFlush`/`glFinish` behavior
on GLES. This PR leaves that backend-specific visibility behavior
intact.

## Correctness invariant

When a transition to GPU-disabled returns, every tracked image-upload
Metal command buffer committed while the GPU `SyncSwitch` was enabled
must have reached either:

- the scheduled state; or
- a terminal completed/failed state from which it cannot later begin GPU
work.

At the same time, ordinary foreground image uploads must not
synchronously wait for scheduling or completion.

## How the new ordering works

- **Foreground:** while holding the enabled/shared `SyncSwitch` lock,
install the Metal handlers, commit the upload, register its receipt, and
return.
- **GPU disable:** the exclusive switch transition first waits for those
shared handlers to finish, then synchronously drains every pending
receipt before returning.

The `SyncSwitch` lock ordering is the proof that registration cannot be
missed: `SetSwitch(true)` cannot acquire its unique lock until an
upload's shared handler has committed and registered its receipt.
Observer notification is synchronous, so the disable transition cannot
return before those receipts are safe. Receipt waiting happens outside
both the switch lock and tracker mutex.

## Implementation

- `CommandBufferSchedulingReceipt` provides a thread-safe, idempotent
scheduled-or-terminal signal. Scheduling, completion, and failure all
wake waiters, and callbacks run outside its mutex.
- `CommandBufferMTL` installs scheduled and completed handlers before
`commit` and returns the receipt without waiting or creating a retain
cycle.
- `PendingImageUploadScheduleTracker` tracks every outstanding upload
receipt, handles callback/registration races, uses weak callback state,
and waits only after releasing its mutex.
- `ContextMTL` drains the tracker when GPU access is disabled while
preserving the existing deferred-task behavior when it is enabled again.
- `SubmitWithReceipt` replaces the generic `block_on_schedule` Boolean.
Metal returns a real receipt; Vulkan and generic fallbacks preserve
their existing submission paths; GLES fence and flush/finish behavior is
unchanged.
- Focused traces cover foreground submission, lifecycle drains, pending
counts, and individual waits.

## Positive impact

This changes where synchronization is paid, rather than weakening it:

- image-decode workers return after commit and receipt registration
instead of stalling on Metal scheduling for every image;
- batches become ready sooner, with the largest gains for generated
pixels, camera buffers, and other upload-heavy sources;
- ordinary JPEG and disk-cached image paths also show repeatable
end-to-end gains;
- the rare GPU-disable transition waits only for image uploads that are
still unscheduled, and only until scheduled/terminal—not until GPU
completion;
- the original backgrounding guarantee is retained and GLES behavior is
unchanged.

There is no Dart or public Flutter framework API change.

## Performance

All measurements below used profile builds with Impeller on the same
wired physical iPhone 14 Pro Max running iOS 26.5. Lower is better.

### Existing `large_image_changer` macrobenchmark: 30-run A/B/A

The repository macrobenchmark was run 10 times with the baseline, 10
times with this implementation, and 10 more times after returning to the
baseline. The same trace surrounded the submission block in both
variants.

| Metric | Combined baseline | This PR | Change |
|---|---:|---:|---:|
| Foreground submit p50 | 2.098 ms | 0.024 ms | **−98.86%** |
| Foreground submit p99 | 2.855 ms | 0.056 ms | **−98.04%** |
| Complete `UploadTextureToPrivate` p50 | 2.654 ms | 0.559 ms |
**−78.94%** |
| Complete `UploadTextureToPrivate` p99 | 4.355 ms | 0.925 ms |
**−78.76%** |

The direct trace confirms that the native scheduling wait leaves the
foreground critical path.

### Image-upload stress suite: repeated A/B/A

A standalone A/B/A suite used three runs per engine and 120 image
uploads per run. End-to-end values are run medians; native p50 values
pool 360 events.

| Workload | Median time until 120 images are ready | Synchronous
`UploadTextureToPrivate` p50 |
|---|---:|---:|
| Bundled JPEG + `ResizeImage` | 768.692 → 726.815 ms (**−5.4%**) |
505.5 → 60.0 µs (**−88.1%**) |
| Preseeded `CachedNetworkImageProvider` + `ResizeImage` | 847.037 →
799.973 ms (**−5.6%**) | 477.0 → 56.0 µs (**−88.3%**) |
| Raw 512×512 RGBA + resize | 111.705 → 70.902 ms (**−36.5%**) | 547.5 →
141.0 µs (**−74.2%**) |

Every changed run was faster than every baseline run. The raw-pixel
result is an upload-heavy upper bound; the 5.4–5.6% JPEG results are
more representative. Native percentages measure removed synchronous CPU
blocking, not faster GPU copy throughput.

## Standalone benchmark reproduction

This self-contained sample reproduces the upload-heavy raw-pixel
workload. Run it in profile mode against baseline and PR engines on the
same iOS device and compare `elapsed_micros` plus the
`UploadTextureToPrivate` trace spans.

<details>
<summary>Expand standalone Flutter benchmark source and
commands</summary>

Create a project with `flutter create --platforms=ios
impeller_upload_benchmark`, then replace/add the following files.

### `pubspec.yaml`

```yaml
name: impeller_upload_benchmark
publish_to: none

environment:
  sdk: '>=3.4.0 <4.0.0'

dependencies:
  flutter:
    sdk: flutter

dev_dependencies:
  flutter_driver:
    sdk: flutter
  flutter_test:
    sdk: flutter
  test: any

flutter:
  uses-material-design: true
```

### `lib/main.dart`

```dart
import 'dart:convert';
import 'dart:ui' as ui;

import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';

final GlobalKey<BenchmarkPageState> benchmarkKey =
    GlobalKey<BenchmarkPageState>();

void main() {
  WidgetsFlutterBinding.ensureInitialized();
  runApp(const BenchmarkApp());
}

Future<String> handleDriverMessage(String? message) async {
  switch (message) {
    case 'warmup':
      await benchmarkKey.currentState!.warmup();
      return 'ready';
    case 'run':
      return jsonEncode(await benchmarkKey.currentState!.runBenchmark());
    default:
      throw UnsupportedError('Unsupported driver message: $message');
  }
}

class BenchmarkApp extends StatelessWidget {
  const BenchmarkApp({super.key});

  @OverRide
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Scaffold(
        body: SafeArea(child: BenchmarkPage(key: benchmarkKey)),
      ),
    );
  }
}

class BenchmarkPage extends StatefulWidget {
  const BenchmarkPage({super.key});

  @OverRide
  State<BenchmarkPage> createState() => BenchmarkPageState();
}

class BenchmarkPageState extends State<BenchmarkPage> {
  static const int imageCount = 120;
  static const int sourceWidth = 512;
  static const int sourceHeight = 512;
  static const int targetWidth = 860;
  static const int targetHeight = 1864;

  late final Uint8List _pixels;
  late List<ImageProvider<Object>> _providers;
  int _generation = 0;

  @OverRide
  void initState() {
    super.initState();
    _pixels = _makePixels();
    _providers = _makeProviders();
  }

  Uint8List _makePixels() {
    final Uint8List pixels = Uint8List(sourceWidth * sourceHeight * 4);
    for (var offset = 0; offset < pixels.length; offset += 4) {
      final int pixel = offset ~/ 4;
      pixels[offset] = pixel & 0xFF;
      pixels[offset + 1] = (pixel >> 8) & 0xFF;
      pixels[offset + 2] = 0x80;
      pixels[offset + 3] = 0xFF;
    }
    return pixels;
  }

  List<ImageProvider<Object>> _makeProviders() {
    return List<ImageProvider<Object>>.generate(
      imageCount,
      (int index) => RawPixelsImageProvider(
        id: _generation * imageCount + index,
        pixels: _pixels,
        width: sourceWidth,
        height: sourceHeight,
        targetWidth: targetWidth,
        targetHeight: targetHeight,
      ),
      growable: false,
    );
  }

  Future<void> _reset() async {
    PaintingBinding.instance.imageCache
      ..clear()
      ..clearLiveImages();
    _generation += 1;
    _providers = _makeProviders();
    setState(() {});
    await WidgetsBinding.instance.endOfFrame;
  }

  Future<void> warmup() async {
    for (final ImageProvider<Object> provider in _providers.take(40)) {
      await precacheImage(provider, context);
    }
    await _reset();
    await Future<void>.delayed(const Duration(milliseconds: 500));
  }

  Future<Map<String, Object>> runBenchmark() async {
    await _reset();
    if (!mounted) {
      throw StateError('Benchmark page was disposed');
    }
    final Stopwatch stopwatch = Stopwatch()..start();
    for (final ImageProvider<Object> provider in _providers) {
      await precacheImage(provider, context);
    }
    stopwatch.stop();
    return <String, Object>{
      'image_count': imageCount,
      'elapsed_micros': stopwatch.elapsedMicroseconds,
    };
  }

  @OverRide
  Widget build(BuildContext context) {
    return Center(
      key: const ValueKey<String>('benchmark-ready'),
      child: Text('Ready (generation $_generation)'),
    );
  }
}

@immutable
class RawPixelsImageProvider extends ImageProvider<RawPixelsImageProvider> {
  const RawPixelsImageProvider({
    required this.id,
    required this.pixels,
    required this.width,
    required this.height,
    required this.targetWidth,
    required this.targetHeight,
  });

  final int id;
  final Uint8List pixels;
  final int width;
  final int height;
  final int targetWidth;
  final int targetHeight;

  @OverRide
  Future<RawPixelsImageProvider> obtainKey(ImageConfiguration configuration) {
    return SynchronousFuture<RawPixelsImageProvider>(this);
  }

  @OverRide
  ImageStreamCompleter loadImage(
    RawPixelsImageProvider key,
    ImageDecoderCallback decode,
  ) {
    return MultiFrameImageStreamCompleter(
      codec: _loadCodec(),
      scale: 1,
      debugLabel: 'RawPixelsImageProvider($id)',
    );
  }

  Future<ui.Codec> _loadCodec() async {
    final ui.ImmutableBuffer buffer = await ui.ImmutableBuffer.fromUint8List(
      pixels,
    );
    final ui.ImageDescriptor descriptor = ui.ImageDescriptor.raw(
      buffer,
      width: width,
      height: height,
      pixelFormat: ui.PixelFormat.rgba8888,
    );
    try {
      return await descriptor.instantiateCodec(
        targetWidth: targetWidth,
        targetHeight: targetHeight,
      );
    } finally {
      descriptor.dispose();
      buffer.dispose();
    }
  }
}
```

### `test_driver/app.dart`

```dart
import 'package:flutter_driver/driver_extension.dart';
import 'package:impeller_upload_benchmark/main.dart' as app;

void main() {
  enableFlutterDriverExtension(handler: app.handleDriverMessage);
  app.main();
}
```

### `test_driver/benchmark_test.dart`

```dart
import 'dart:convert';
import 'dart:io';

import 'package:flutter_driver/flutter_driver.dart';
import 'package:test/test.dart' hide TypeMatcher, isInstanceOf;

Future<void> main() async {
  late FlutterDriver driver;

  setUpAll(() async {
    driver = await FlutterDriver.connect();
    await driver.waitUntilFirstFrameRasterized();
    await driver.waitFor(find.byValueKey('benchmark-ready'));
  });

  tearDownAll(() async {
    await driver.close();
  });

  test('measure raw image upload scheduling', () async {
    expect(await driver.requestData('warmup'), 'ready');
    await driver.forceGC();

    late String response;
    final Timeline timeline = await driver.traceAction(() async {
      response = await driver.requestData(
        'run',
        timeout: const Duration(minutes: 5),
      );
    }, streams: const <TimelineStream>[TimelineStream.embedder]);

    final Map<String, dynamic> result =
        jsonDecode(response) as Map<String, dynamic>;
    expect(result['image_count'], 120);
    expect(result['elapsed_micros'], greaterThan(0));

    await Directory('build').create(recursive: true);
    await File('build/raw_upload_result.json')
        .writeAsString(const JsonEncoder.withIndent('  ').convert(result));
    await TimelineSummary.summarize(timeline)
        .writeTimelineToFile('raw_upload', pretty: true, includeSummary: false);
    // Also keep the value visible in CI/terminal logs.
    // ignore: avoid_print
    print(response);
  }, timeout: Timeout.none);
}
```

### Run against each local engine

```sh
flutter pub get

flutter drive \
  --profile \
  -d <device-id> \
  --local-engine-src-path=<flutter-checkout>/engine/src \
  --local-engine=<baseline-ios-profile-output-name> \
  --local-engine-host=<host-profile-output-name> \
  -t test_driver/app.dart \
  --driver test_driver/benchmark_test.dart

# Repeat with only --local-engine changed to the PR engine output.
```

Each run writes:

```text
build/raw_upload_result.json
build/raw_upload.timeline.json
```

Use at least three runs per engine and return to the baseline after the
changed phase. In the timeline, pair `B`/`E` events named
`UploadTextureToPrivate` by PID and thread ID before calculating
percentiles.

</details>


## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

---------

Co-authored-by: gaaclarke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine flutter/engine related. See also e: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants