Skip to content
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.

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

[web] Make Canvaskit's malloc more useful #38130

Merged
merged 3 commits into from
Dec 14, 2022
Merged

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Dec 7, 2022

Open up Canvaskit's malloc to be used for other types of lists, not just float32.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Dec 7, 2022
@mdebbar mdebbar force-pushed the ck_malloc branch 2 times, most recently from 58198e8 to 5e7c3b7 Compare December 13, 2022 16:08
@mdebbar mdebbar marked this pull request as ready for review December 13, 2022 16:17
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm


/// Allocates a [Float32List] backed by WASM memory, managed by
/// a [SkFloat32List].
///
/// To free the allocated array use [freeFloat32List].
/// To free the allocated array use [freeList].
SkFloat32List mallocFloat32List(int size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's document if size is in bytes or element count. Also, if it's the latter, then perhaps length is a more idiomatic name for the parameter (and I think sizeInBytes is more common for the other one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check what CanvasKit calls the parameter.

///
/// The [list] is no longer usable after calling this function.
///
/// Use this function to free lists owned by the engine.
@JS('window.flutterCanvasKit.Free')
external void freeFloat32List(SkFloat32List list);
external void freeList(_MallocObj list);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's strange for a public function to take a private type. Can _MallocObj be public?

Also, nit: avoid abbreviations - MallocObject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll make it public.

As for the abbreviation, I used whatever name CanvasKit uses: https://github.com/google/skia/blob/0e95fd2482f8a456b2bcabf350efd51a042c3b97/modules/canvaskit/npm_build/types/index.d.ts#L786

///
/// The [list] is no longer usable after calling this function.
///
/// Use this function to free lists owned by the engine.
@JS('window.flutterCanvasKit.Free')
external void freeFloat32List(SkFloat32List list);
external void freeList(_MallocObj list);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need List in the name? In the future, we could extend the API to include other malloc-ed types, including non-List kinds, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need List in the name. I'll remove it.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Dec 14, 2022

auto label is removed for flutter/engine, pr: 38130, due to - The status or check suite Linux Web Framework tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2022
@auto-submit auto-submit bot merged commit d1533d1 into flutter:main Dec 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 15, 2022
…117109)

* 1a1b1feee Roll Skia from 537e1e8c1ca6 to 729ccbfb87bc (7 revisions) (flutter/engine#38277)

* 3b18821e1 Roll Fuchsia Linux SDK from A0jnUUORf2LQu1z2V... to e2lfUFBW5ddtTZBbw... (flutter/engine#38280)

* beb94ea2c Roll Skia from 729ccbfb87bc to 3171deabd88a (4 revisions) (flutter/engine#38279)

* 8f9071ab9 Roll Fuchsia Mac SDK from FQQdl8AGAsALFniHl... to u-tC0QEGUT4xQ4KOo... (flutter/engine#38282)

* aa78cd8d6 add link to website (flutter/engine#38273)

* 955447b35 pylint all Python scripts under testing/ (flutter/engine#38268)

* 3f22e1958 [web] correct float count in runtime effect (flutter/engine#38288)

* 479bb736f Fix issues related to keyboard inset (flutter/engine#37719)

* 6cd85616b [macOS] Refactor rendering infrastructure (flutter/engine#37789)

* d1533d12b [web] Make Canvaskit's malloc more useful (flutter/engine#38130)

* db5605ea7 Fix new `unnecessary_parenthesis` diagnostics. (flutter/engine#38291)
loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Dec 16, 2022
* [web] Make Canvaskit's malloc more useful

* address review comments
loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Jan 3, 2023
* [web] Make Canvaskit's malloc more useful

* address review comments
@mdebbar mdebbar deleted the ck_malloc branch January 17, 2023 18:11
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#117109)

* 1a1b1feee Roll Skia from 537e1e8c1ca6 to 729ccbfb87bc (7 revisions) (flutter/engine#38277)

* 3b18821e1 Roll Fuchsia Linux SDK from A0jnUUORf2LQu1z2V... to e2lfUFBW5ddtTZBbw... (flutter/engine#38280)

* beb94ea2c Roll Skia from 729ccbfb87bc to 3171deabd88a (4 revisions) (flutter/engine#38279)

* 8f9071ab9 Roll Fuchsia Mac SDK from FQQdl8AGAsALFniHl... to u-tC0QEGUT4xQ4KOo... (flutter/engine#38282)

* aa78cd8d6 add link to website (flutter/engine#38273)

* 955447b35 pylint all Python scripts under testing/ (flutter/engine#38268)

* 3f22e1958 [web] correct float count in runtime effect (flutter/engine#38288)

* 479bb736f Fix issues related to keyboard inset (flutter/engine#37719)

* 6cd85616b [macOS] Refactor rendering infrastructure (flutter/engine#37789)

* d1533d12b [web] Make Canvaskit's malloc more useful (flutter/engine#38130)

* db5605ea7 Fix new `unnecessary_parenthesis` diagnostics. (flutter/engine#38291)
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 platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants