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 2, 2020

Description

Without this, the GC thinks that Vertices are super cheap. Allocating a lot of them (e.g. new ones each frame) uses tons of memory - on smaller devices, enough memory to OOM die.

Including this in the total picture memory (like in #18706) makes this further use less memory. Doing that fixes part of flutter/flutter#58438

/cc @mehmetf - this may help customer: money with some of their Rive/Flare concerns, since Flare uses this API.
/cc @luigi-rosso @cmkweber

Related Issues

Fixes flutter/flutter#54762
Possibly helps 2d-inc/Flare-Flutter#249

Tests

TODO

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 marked this pull request as ready for review June 3, 2020 02:01
@auto-assign auto-assign bot requested a review from jason-simmons June 3, 2020 02:02
@dnfield dnfield requested review from chinmaygarde and removed request for jason-simmons June 3, 2020 02:02
@dnfield
Copy link
Contributor Author

dnfield commented Jun 3, 2020

The only failure for this is fixed on master. I'd rather not rebase and find out another test flaked. Merging on red.

@dnfield dnfield merged commit 25054fb into flutter:master Jun 3, 2020
@dnfield dnfield deleted the vertices_mem branch June 3, 2020 17:35
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2020
@luigi-rosso
Copy link
Contributor

Sorry for commenting on this so late, but I'm curious if there's an opportunity here to re-use existing TypedData arrays (like the Float32List for the vertex positions) by allowing changing the contents of the buffer instead of allocating new ones each frame. Perhaps some way to notify the Vertices that the buffer has changed and it needs to internally resync with Skia? @dnfield Let me know if you think it's worth opening an issue for this.

@dnfield
Copy link
Contributor Author

dnfield commented Jul 10, 2020

Do the arrays resize per frame? If so then it's probably better to just allocate new ones and let the GC do its thing, because resizing the buffers would cause a lot of thrash anyway. If not then it's probably worth exploring.

@dnfield
Copy link
Contributor Author

dnfield commented Jul 10, 2020

Alternatively, if the array has a (reasonable) upperbound, it could make sense to have an array and a length parameter for how much of the array you actually want to use.

@luigi-rosso
Copy link
Contributor

luigi-rosso commented Jul 10, 2020

That's a good idea, but they currently do not resize. We only update the vertices when the animation animates them which currently involves moving (just changing x/y values) them. It'd also only be the position buffer that updates, the UV and index buffers are static throughout.

@cmkweber
Copy link

I believe Skia is working on per vertex data (SkVertices::Attribute) that can be exposed to shaders.
This functionality would be great to see in flutter once it's completed.
This would allow for a constant size buffer but dynamic contents.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Leak within Vertices

6 participants