Skip to content

Optimize file transfer when using proxied devices. #139968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
auto-submit merged 2 commits into from
Dec 14, 2023
Merged

Optimize file transfer when using proxied devices. #139968

auto-submit merged 2 commits into from
Dec 14, 2023

Conversation

@chingjun
Copy link
Contributor

@chingjun chingjun commented Dec 12, 2023

List of changes:

  1. Optimizations in FileTransfer. a. Use stream.forEach instead of await for. b. Type cast List<int> to Uint8List instead of using Uint8List.fromList results in (presumably) fewer copy and faster execution. c. Iterate through Uint8List with regular for loop instead of for-in loop.
  2. Precache the block hashes of a file, and reuse it on subsequent runs.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 12, 2023
christopherfujino
christopherfujino reviewed Dec 12, 2023
View reviewed changes
packages/flutter_tools/lib/src/proxied_devices/file_transfer.dart Outdated
Copy link
Contributor

@christopherfujino christopherfujino Dec 12, 2023

Choose a reason for hiding this comment

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

Why would this sometimes be a Uint8List and sometimes not? is it OS dependent?

Copy link
Contributor

@christopherfujino christopherfujino Dec 12, 2023

Choose a reason for hiding this comment

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

Although, I suppose it doesn't really matter, before we would ALWAYS call Uint8List.fromList(chunk)

Copy link
Contributor Author

@chingjun chingjun Dec 12, 2023

Choose a reason for hiding this comment

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

It's not OS dependent and it should always be a Uint8List.

The File.openRead() function returns a Stream<List<int>>, but the underlying type is actually a Uint8List (https://github.com/dart-lang/sdk/blob/main/sdk/lib/io/file_impl.dart#L29)

I don't know why the types do not match, though I think it's mainly for historical reason. But since it's not reflected from the API, I'm treating this as an implementation detail.

I wrote it this way mostly to satisfy Dart's type checking. It's faster than Uint8List.fromList because fromList always copy the bytes, and it's safer than a direct type cast because that would depend on the implementation details. I fully expect it to always hit the first branch though :)

Copy link
Contributor

@christopherfujino christopherfujino Dec 12, 2023

Choose a reason for hiding this comment

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

SGTM, thanks for the explanation, was mostly curious.

christopherfujino
christopherfujino approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@chingjun chingjun added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 12, 2023
@chingjun
Optimize file transfer when using proxied devices.
e9f8fdb 
List of changes:
1. Optimizations in FileTransfer.
   a. Use `stream.forEach` instead of `await for`.
   b. Type cast `List<int>` to `Uint8List` instead of using
   `Uint8List.fromList` results in (presumably) fewer copy and faster
   execution.
   c. Iterate through `Uint8List` with regular for loop instead of
   for-in loop.
2. Precache the block hashes of a file, and reuse it on subsequent runs.
@chingjun
Remove extra space characters
ba7b7a9
@chingjun chingjun force-pushed the file_transfer_optimize branch from 7ff8e78 to ba7b7a9 Compare December 13, 2023 05:41
@auto-submit auto-submit bot merged commit c1caa24 into flutter:master Dec 14, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 14, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023
@engine-flutter-autoroll
c1caa24 Optimize file transfer when using proxied devices. (flutter/f…
00f6407 
…lutter#139968)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 14, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023
@engine-flutter-autoroll
c1caa24 Optimize file transfer when using proxied devices. (flutter/f…
8f60c21 
…lutter#139968)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 14, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023
@engine-flutter-autoroll
c1caa24 Optimize file transfer when using proxied devices. (flutter/f…
a92e310 
…lutter#139968)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 14, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 14, 2023
@engine-flutter-autoroll
c1caa24 Optimize file transfer when using proxied devices. (flutter/f…
6faf97d 
…lutter#139968)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Dec 14, 2023
@engine-flutter-autoroll
Roll Flutter from 11a9cb7 to a51e33a (30 revisions) (#5680)
ff71954 
flutter/flutter@11a9cb7...a51e33a

2023-12-14 [email protected] Roll Flutter Engine from 223f4b2465dd to caf33276468b (1 revision) (flutter/flutter#140156)
2023-12-14 [email protected] Roll Packages from b5958e2 to 1151191 (10 revisions) (flutter/flutter#140154)
2023-12-14 [email protected] Roll Flutter Engine from a3f9393f9591 to 223f4b2465dd (1 revision) (flutter/flutter#140151)
2023-12-14 [email protected] Roll Flutter Engine from 913446eca57c to a3f9393f9591 (2 revisions) (flutter/flutter#140144)
2023-12-14 [email protected] Roll Flutter Engine from 923f9e29d4b5 to 913446eca57c (1 revision) (flutter/flutter#140132)
2023-12-14 [email protected] Roll Flutter Engine from 9f7004e3e30e to 923f9e29d4b5 (7 revisions) (flutter/flutter#140130)
2023-12-14 [email protected] Add self back to CODEOWNERS (flutter/flutter#140080)
2023-12-14 [email protected] Adapt wording for required Android SDK for plugins (flutter/flutter#140043)
2023-12-14 [email protected] [reland] Support conditional bundling of assets based on `--flavor` (flutter/flutter#139834)
2023-12-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 9f7004e3e30e to 45b95f264d63 (1 revision)" (flutter/flutter#140123)
2023-12-14 [email protected] [deps] update Android SDK to 34 (flutter/flutter#138183)
2023-12-14 [email protected] Roll Flutter Engine from 9f7004e3e30e to 45b95f264d63 (1 revision) (flutter/flutter#140108)
2023-12-14 [email protected] Catch `Stopwatch` with static analysis (flutter/flutter#140019)
2023-12-14 [email protected] Overlay supports unconstrained environments (flutter/flutter#139513)
2023-12-14 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.10 to 3.22.11 (flutter/flutter#140087)
2023-12-14 [email protected] Remove deprecated `ThemeData.selectedRowColor` (flutter/flutter#139080)
2023-12-14 [email protected] Unpin mac_toolchain version (flutter/flutter#139938)
2023-12-14 [email protected] Optimize file transfer when using proxied devices. (flutter/flutter#139968)
2023-12-14 [email protected] Add commonly used parameter names (flutter/flutter#140027)
2023-12-13 [email protected] Roll Flutter Engine from fc3267724e1b to 9f7004e3e30e (4 revisions) (flutter/flutter#140090)
2023-12-13 [email protected] [Windows] Remove header guard from generated key map (flutter/flutter#140082)
2023-12-13 [email protected] Do not use project in do last (flutter/flutter#139325)
2023-12-13 [email protected] Roll pub packages (flutter/flutter#140024)
2023-12-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Warn when Gradle plugins are applied using the legacy "apply script method" way" (flutter/flutter#140102)
2023-12-13 [email protected] Swap iOS back button icon in Material app bar (flutter/flutter#134754)
2023-12-13 [email protected] Roll Packages from 80aa46a to b5958e2 (10 revisions) (flutter/flutter#140069)
2023-12-13 [email protected] Roll Flutter Engine from 9039ac78cf03 to fc3267724e1b (26 revisions) (flutter/flutter#140084)
2023-12-13 [email protected] Warn when Gradle plugins are applied using the legacy "apply script method" way (flutter/flutter#139690)
2023-12-13 [email protected] Add self as bundler dependabot reviewer (flutter/flutter#140081)
2023-12-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal" (flutter/flutter#140085)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
c1caa24 Optimize file transfer when using proxied devices. (flutter/f…
37a12c6 
…lutter#139968)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chingjun @christopherfujino