Skip to content

performance: Override .elementAt in CachingIterable - #152477

Merged
auto-submit[bot] merged 1 commit into
flutter:masterfrom
plammens:patch-1
Nov 1, 2024
Merged

auto-submit[bot] merged 1 commit into
flutter:masterfrom
plammens:patch-1

Conversation

@plammens

@plammens plammens commented Jul 29, 2024 •

Copy link
Copy Markdown
Contributor

Add a more efficient override of Iterable.elementAt in CachingIterable.

Closes #152476

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'm not sure if I should add a documentation comment as it would override Iterable.elementAt's documentation, but functionally it's the same.
  • I added new tests to check the change I am making, or this PR is test-exempt. I only added a functional test to check the implementation's correctness, because I can't think of a robust way to test the performance aspect, namely the random access.
  • I followed the breaking change policy and added Data Driven Fixes where supported.
  • 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 framework flutter/packages/flutter repository. See also f: labels. label Jul 29, 2024
@chunhtai

chunhtai commented Oct 1, 2024

Copy link
Copy Markdown
Contributor

(triage) Hi @plammens what is the status of this pr? if it is ready for review, can you convert it to a open pr?

@plammens

plammens commented Oct 2, 2024

Copy link
Copy Markdown
Contributor Author

(triage) Hi @plammens what is the status of this pr? if it is ready for review, can you convert it to a open pr?

Sure, if you like the proposed implementation I can go ahead and open it! I'll try to go through the checklist items ASAP.

@plammens
plammens marked this pull request as ready for review October 2, 2024 20:56
@flutter-dashboard

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. 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.

@plammens

plammens commented Oct 5, 2024

Copy link
Copy Markdown
Contributor Author

@chunhtai Should be ready for review now.

@chunhtai
chunhtai self-requested a review October 8, 2024 19:13
Comment thread packages/flutter/lib/src/foundation/basic_types.dart
Comment thread packages/flutter/test/foundation/caching_iterable_test.dart

@chunhtai chunhtai 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.

LGTM

@chunhtai

Copy link
Copy Markdown
Contributor

looks like there is some ci error

00:11 +315 ~1 -1: /Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/foundation/caching_iterable_test.dart: The Caching Iterable: elementAt correctness [E]
RangeError (index): Invalid value: Not greater than or equal to 0: -1
dart:core RangeError.checkNotNegative
package:flutter/src/foundation/basic_types.dart 186:16 CachingIterable.elementAt
test/foundation/caching_iterable_test.dart 115:21 main.

@nate-thegrate nate-thegrate 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.

LGTM, thank you!

expect(yieldCount, equals(5));
});

test('The Caching Iterable: elementAt correctness', () {

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.

I think "The Caching Iterable" looks kinda silly, but since it's following a pattern from other tests in this file it's all good 👍

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.

Not really in the scope of this PR, but we should probably migrate to the new API at some point:

- class CachingIterable<E> extends IterableBase<E> {
+ class CachingIterable<E> with Iterable<E> {

Comment thread packages/flutter/test/foundation/caching_iterable_test.dart Outdated
Comment thread packages/flutter/test/foundation/caching_iterable_test.dart Outdated
@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2024
@plammens

Copy link
Copy Markdown
Contributor Author

looks like there is some ci error

00:11 +315 ~1 -1: /Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/foundation/caching_iterable_test.dart: The Caching Iterable: elementAt correctness [E] RangeError (index): Invalid value: Not greater than or equal to 0: -1 dart:core RangeError.checkNotNegative package:flutter/src/foundation/basic_types.dart 186:16 CachingIterable.elementAt test/foundation/caching_iterable_test.dart 115:21 main.

Sorry, I didn't manage to get the tests running locally so I can't test before pushing! Hopefully it's all good now.

@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2024
@auto-submit

auto-submit Bot commented Oct 31, 2024

Copy link
Copy Markdown
Contributor

auto label is removed for flutter/flutter/152477, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chunhtai

Copy link
Copy Markdown
Contributor

google test failure doesn't seem related, rerunning to see if it passes

auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Nov 1, 2024
flutter/flutter@0fe6153...f86b777

2024-11-01 [email protected] Roll Packages from 7cc1caa to 796afa3 (15 revisions) (flutter/flutter#158003)
2024-11-01 [email protected] Marks Linux_pixel_7pro service_extensions_test to be flaky (flutter/flutter#157853)
2024-11-01 [email protected] Roll Flutter Engine from 0a0d5c9be6ff to 3a090b46dd35 (1 revision) (flutter/flutter#157994)
2024-11-01 [email protected] Roll Flutter Engine from bacc5e1e73b7 to 0a0d5c9be6ff (3 revisions) (flutter/flutter#157991)
2024-11-01 [email protected] Add test for `interactive_viewer.transformation_controller.0.dart` (flutter/flutter#157986)
2024-11-01 [email protected] Roll Flutter Engine from d7e928911ac2 to bacc5e1e73b7 (1 revision) (flutter/flutter#157982)
2024-11-01 [email protected] Add test for `notification.0.dart` (flutter/flutter#157909)
2024-11-01 [email protected] performance: Override .elementAt in CachingIterable (flutter/flutter#152477)
2024-11-01 [email protected] Roll Flutter Engine from cd46383cd55e to d7e928911ac2 (4 revisions) (flutter/flutter#157978)
2024-11-01 [email protected] Roll Flutter Engine from bb77cf867aef to cd46383cd55e (11 revisions) (flutter/flutter#157972)
2024-11-01 [email protected] Add a warning/additional handlers for parsing`synthetic-package`. (flutter/flutter#157934)
2024-10-31 [email protected] Roll Flutter Engine from f2154ef3e31c to bb77cf867aef (6 revisions) (flutter/flutter#157960)
2024-10-31 [email protected] Renames `injectBuildTimePluginFilesForWebPlatform` and removes unused named parameter. (flutter/flutter#157944)
2024-10-31 [email protected] [flutter_driver] use mostly public screenshot API. (flutter/flutter#157888)
2024-10-31 [email protected] Made insetPadding configurable for Date Picker Dialog (flutter/flutter#155651)
2024-10-31 [email protected] Fix showSnackBar can't access useMaterial3 from the theme (flutter/flutter#157707)

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] 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 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
creatorpiyush pushed a commit to creatorpiyush/packages that referenced this pull request Jun 10, 2026
flutter/flutter@0fe6153...f86b777

2024-11-01 [email protected] Roll Packages from 7cc1caa to 796afa3 (15 revisions) (flutter/flutter#158003)
2024-11-01 [email protected] Marks Linux_pixel_7pro service_extensions_test to be flaky (flutter/flutter#157853)
2024-11-01 [email protected] Roll Flutter Engine from 0a0d5c9be6ff to 3a090b46dd35 (1 revision) (flutter/flutter#157994)
2024-11-01 [email protected] Roll Flutter Engine from bacc5e1e73b7 to 0a0d5c9be6ff (3 revisions) (flutter/flutter#157991)
2024-11-01 [email protected] Add test for `interactive_viewer.transformation_controller.0.dart` (flutter/flutter#157986)
2024-11-01 [email protected] Roll Flutter Engine from d7e928911ac2 to bacc5e1e73b7 (1 revision) (flutter/flutter#157982)
2024-11-01 [email protected] Add test for `notification.0.dart` (flutter/flutter#157909)
2024-11-01 [email protected] performance: Override .elementAt in CachingIterable (flutter/flutter#152477)
2024-11-01 [email protected] Roll Flutter Engine from cd46383cd55e to d7e928911ac2 (4 revisions) (flutter/flutter#157978)
2024-11-01 [email protected] Roll Flutter Engine from bb77cf867aef to cd46383cd55e (11 revisions) (flutter/flutter#157972)
2024-11-01 [email protected] Add a warning/additional handlers for parsing`synthetic-package`. (flutter/flutter#157934)
2024-10-31 [email protected] Roll Flutter Engine from f2154ef3e31c to bb77cf867aef (6 revisions) (flutter/flutter#157960)
2024-10-31 [email protected] Renames `injectBuildTimePluginFilesForWebPlatform` and removes unused named parameter. (flutter/flutter#157944)
2024-10-31 [email protected] [flutter_driver] use mostly public screenshot API. (flutter/flutter#157888)
2024-10-31 [email protected] Made insetPadding configurable for Date Picker Dialog (flutter/flutter#155651)
2024-10-31 [email protected] Fix showSnackBar can't access useMaterial3 from the theme (flutter/flutter#157707)

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] 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
bisor0627 pushed a commit to bisor0627/packages that referenced this pull request Jun 19, 2026
flutter/flutter@0fe6153...f86b777

2024-11-01 [email protected] Roll Packages from 2a42664 to fbbb277 (15 revisions) (flutter/flutter#158003)
2024-11-01 [email protected] Marks Linux_pixel_7pro service_extensions_test to be flaky (flutter/flutter#157853)
2024-11-01 [email protected] Roll Flutter Engine from 0a0d5c9be6ff to 3a090b46dd35 (1 revision) (flutter/flutter#157994)
2024-11-01 [email protected] Roll Flutter Engine from bacc5e1e73b7 to 0a0d5c9be6ff (3 revisions) (flutter/flutter#157991)
2024-11-01 [email protected] Add test for `interactive_viewer.transformation_controller.0.dart` (flutter/flutter#157986)
2024-11-01 [email protected] Roll Flutter Engine from d7e928911ac2 to bacc5e1e73b7 (1 revision) (flutter/flutter#157982)
2024-11-01 [email protected] Add test for `notification.0.dart` (flutter/flutter#157909)
2024-11-01 [email protected] performance: Override .elementAt in CachingIterable (flutter/flutter#152477)
2024-11-01 [email protected] Roll Flutter Engine from cd46383cd55e to d7e928911ac2 (4 revisions) (flutter/flutter#157978)
2024-11-01 [email protected] Roll Flutter Engine from bb77cf867aef to cd46383cd55e (11 revisions) (flutter/flutter#157972)
2024-11-01 [email protected] Add a warning/additional handlers for parsing`synthetic-package`. (flutter/flutter#157934)
2024-10-31 [email protected] Roll Flutter Engine from f2154ef3e31c to bb77cf867aef (6 revisions) (flutter/flutter#157960)
2024-10-31 [email protected] Renames `injectBuildTimePluginFilesForWebPlatform` and removes unused named parameter. (flutter/flutter#157944)
2024-10-31 [email protected] [flutter_driver] use mostly public screenshot API. (flutter/flutter#157888)
2024-10-31 [email protected] Made insetPadding configurable for Date Picker Dialog (flutter/flutter#155651)
2024-10-31 [email protected] Fix showSnackBar can't access useMaterial3 from the theme (flutter/flutter#157707)

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] 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add .elementAt override to CachingIterable

3 participants