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

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Jun 4, 2020

Description

Path portion of flutter/flutter#58438

GetAllocationSize will only really help in cases where the path is already set up, e.g. for transform, shift, and clone. It will not help as much in the normal case of creating a new path and adding drawing commands to it. For that, we could consider using some new Dart API to update the size.

However, we can at least get better collection around SkPictures - once the path has been drawn to the picture, the picture will use the memory of those path commands.

Related Issues

flutter/flutter#58438

Tests

I added the following tests:

Test that canvas methods involving paths add the size appropriately to the resultant picture.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@dnfield dnfield added the perf: memory Performance issues related to memory label Jun 4, 2020
@dnfield dnfield changed the title Path mem Record path memory usage in SkPictures Jun 4, 2020
@auto-assign auto-assign bot requested a review from GaryQian June 4, 2020 17:21
@chinmaygarde
Copy link
Member

chinmaygarde commented Jun 4, 2020

Now, that Dart_UpdateExternalSize has landed, we should update DartWrappable to have another call like void UpdateAllocationSize(size_t allocation_size) or void AppendAllocationSize(size_t allocation_size) that tracks the size and also calls Dart_UpdateExternalSize. Then, then use that call to keep track of path sizes as they are being built up.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Discussed in chat that updates recommended in to DartWrappable can be made in a subsequent patch. In that case, this is fine in the interim.

@dnfield
Copy link
Contributor Author

dnfield commented Jun 4, 2020

Redness is an infra flake. Going to land this.

@dnfield dnfield merged commit 7e38261 into flutter:master Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2020
@dnfield dnfield deleted the path_mem branch June 5, 2020 16:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes perf: memory Performance issues related to memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants