Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] fixed units for memory measurement #53687

Merged
merged 3 commits into from
Jul 2, 2024
Merged

[Impeller] fixed units for memory measurement #53687

merged 3 commits into from
Jul 2, 2024

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Jul 2, 2024 •
edited
Loading

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@flutter-dashboard
Copy link

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

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.

@gaaclarke gaaclarke requested a review from jonahwilliams July 2, 2024 15:49
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 2, 2024
jonahwilliams
jonahwilliams reviewed Jul 2, 2024
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

But we're reporting the units as MB and not MiB.

@gaaclarke
Copy link
Member Author

But we're reporting the units as MB and not MiB.

Yea, they are unfortunately used interchangeably. MB can mean MiB.

@gaaclarke gaaclarke requested a review from jonahwilliams July 2, 2024 17:41
@jonahwilliams
Copy link
Contributor

But maybe I meant MB and not MiB :)

At any rate, please update allocator_vk too so these are in sync.

jonahwilliams
jonahwilliams reviewed Jul 2, 2024
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

This one: https://github.com/flutter/engine/blob/main/impeller/renderer/backend/vulkan/allocator_vk.cc#L512-L523

@gaaclarke
Copy link
Member Author

But maybe I meant MB and not MiB :)

I thought it mentioned it elsewhere. The convention is that disk space is measured in decimal units, but RAM is measured in power of 2 units 🙃

jonahwilliams
jonahwilliams approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@auto-submit auto-submit bot merged commit b87e3aa into flutter:main Jul 2, 2024
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 2, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 2, 2024
@engine-flutter-autoroll
b87e3aaa2 [Impeller] fixed units for memory measurement (flutter/engi…
526d6fe 
…ne#53687)
@chinmaygarde
Copy link
Member

This doesn't quite sit right. We measure in bytes but the reported units can be anything. I think we can make this better by having typed sizes similar to the chrono variants.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 2, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 2, 2024
@engine-flutter-autoroll
b87e3aaa2 [Impeller] fixed units for memory measurement (flutter/engi…
3fb60bc 
…ne#53687)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 2, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 2, 2024
@engine-flutter-autoroll
b87e3aaa2 [Impeller] fixed units for memory measurement (flutter/engi…
b3da209 
…ne#53687)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
@engine-flutter-autoroll
b87e3aaa2 [Impeller] fixed units for memory measurement (flutter/engi…
64f8e16 
…ne#53687)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 3, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2024
@engine-flutter-autoroll
b87e3aaa2 [Impeller] fixed units for memory measurement (flutter/engi…
3c298d7 
…ne#53687)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 3, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 44278941443e to c5c0c54d6d1d (9 revisions) (#…
f209626 
…151208)

flutter/engine@4427894...c5c0c54

2024-07-03 [email protected] Reland "Output .js files as ES6 modules. (#52023)" (flutter/engine#53688)
2024-07-02 [email protected] Roll Skia from d02998fba957 to 7c69f39fa85b (1 revision) (flutter/engine#53701)
2024-07-02 [email protected] Roll Skia from c1f2dd0fc5f6 to d02998fba957 (2 revisions) (flutter/engine#53699)
2024-07-02 [email protected] Roll Skia from c0ee0e108900 to c1f2dd0fc5f6 (2 revisions) (flutter/engine#53697)
2024-07-02 [email protected] [Impeller] fix typo in setup for fast elliptical rrect blurs (flutter/engine#53673)
2024-07-02 [email protected] Roll Fuchsia GN SDK from RgErspyYHapUO2Spc... to sbh76PYVTMxav4ACT... (flutter/engine#53693)
2024-07-02 [email protected] [Impeller] fixed units for memory measurement (flutter/engine#53687)
2024-07-02 [email protected] [deep link][ios] Update openURL method to reflect the result from framework  (flutter/engine#52643)
2024-07-02 [email protected] Manual roll Dart SDK from c23e58143793 to ffc8bb004a64 (2 revisions) (flutter/engine#53690)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
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 Flutter: 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
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
@engine-flutter-autoroll @victorsanni
Roll Flutter Engine from 44278941443e to c5c0c54d6d1d (9 revisions) (f…
fb251f0 
…lutter#151208)

flutter/engine@4427894...c5c0c54

2024-07-03 [email protected] Reland "Output .js files as ES6 modules. (flutter#52023)" (flutter/engine#53688)
2024-07-02 [email protected] Roll Skia from d02998fba957 to 7c69f39fa85b (1 revision) (flutter/engine#53701)
2024-07-02 [email protected] Roll Skia from c1f2dd0fc5f6 to d02998fba957 (2 revisions) (flutter/engine#53699)
2024-07-02 [email protected] Roll Skia from c0ee0e108900 to c1f2dd0fc5f6 (2 revisions) (flutter/engine#53697)
2024-07-02 [email protected] [Impeller] fix typo in setup for fast elliptical rrect blurs (flutter/engine#53673)
2024-07-02 [email protected] Roll Fuchsia GN SDK from RgErspyYHapUO2Spc... to sbh76PYVTMxav4ACT... (flutter/engine#53693)
2024-07-02 [email protected] [Impeller] fixed units for memory measurement (flutter/engine#53687)
2024-07-02 [email protected] [deep link][ios] Update openURL method to reflect the result from framework  (flutter/engine#52643)
2024-07-02 [email protected] Manual roll Dart SDK from c23e58143793 to ffc8bb004a64 (2 revisions) (flutter/engine#53690)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
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 Flutter: 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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams 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 e: impeller

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gaaclarke @jonahwilliams @chinmaygarde