Delete the Dart functions associated with closure blocks by liamappelbe · Pull Request #1100 · dart-lang/native

liamappelbe · 2024-04-24T01:43:12Z

Changes:

Instead of having a code-gened Dart function registry for each ObjCBlock type, there's now a single registry that lives in package:objective_c
Added a dispose_port field to the captured variables section of the ObjCBlock struct. This is set to the send port of a RawReceivePort that deletes functions from the registry.
Added a native disposeObjCBlockWithClosure function that sends the block ID to the dispose_port. The dispose_helper field of the block is set to this function.
Had to split objective_c_runtime.h into objective_c_runtime_types.h and objective_c_runtime_functions.h to resolve some build issues.
Now that there's native code in package:objective_c, the tests have to build it, so I added test/setup.dart. The ffigen test setup script also has to call this, since ffigen's ObjC tests need it. These tests also need to DynamicLibrary.open that dylib.
Added an example app to package:objective_c
Switched to NSData.dataWithBytes_length_ to fix the tests (see NSData.dataWithBytesNoCopy_length_ is crashing #1107)

Fixes #204

github-actions · 2024-04-24T01:44:35Z

PR Health

Changelog Entry ✔️

Details

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️

Details

File	Coverage
pkgs/ffigen/lib/src/code_generator/objc_block.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart	💔 Not covered
pkgs/objective_c/example/lib/main.dart	💔 Not covered
pkgs/objective_c/lib/objective_c.dart	💔 Not covered
pkgs/objective_c/lib/src/c_bindings_generated.dart	💔 Not covered
pkgs/objective_c/lib/src/internal.dart	💔 Not covered
pkgs/objective_c/lib/src/ns_data.dart	💔 Not covered
pkgs/objective_c/lib/src/objective_c_bindings_generated.dart	💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ✔️

Details

// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/ffi/example/main.dart
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart

Package publish validation ✔️

Details

Package	Version	Status
package:ffi	2.1.2	already published at pub.dev
package:ffigen	12.0.0-wip	WIP (no publish necessary)
package:jni	0.9.0-wip	WIP (no publish necessary)
package:jnigen	0.9.0-wip	WIP (no publish necessary)
package:native_assets_builder	0.6.1	already published at pub.dev
package:native_assets_cli	0.5.4	already published at pub.dev
package:native_toolchain_c	0.4.1	already published at pub.dev
package:objective_c	0.0.1-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

dcharkes

Great! 🚀

LGTM!

dcharkes · 2024-04-26T12:13:04Z

      - name: Install dependencies
        run: flutter pub get
+      - name: Build test dylib
+        run: dart test/setup.dart


Maybe leave todo with link to switch-to-native-assets issue to remove this again.

dcharkes · 2024-04-26T12:20:02Z

+    return MaterialApp(
+      home: Scaffold(
+        body: Center(
+          child: Text(NSString('Hello World!').toString()),


nit: This will rerun this every re-render.

For things that don't change, it's better to use a stateful widget and cache the result. (Feedback from Flutter. It's what we do in the plugin_ffi template in Flutter.)

P.S. Would we keep a Flutter example when switching to native assets later? I guess we can?

Done. Yeah, it probably makes sense to keep the example after the switch.

dcharkes · 2024-04-26T12:21:58Z

 //
 // Generated by `package:ffigen`.
 // ignore_for_file: type=lint
+@ffi.DefaultAsset('objective_c.framework/objective_c')


How does this work currently without support for native assets? By falling back on process lookup?

dcharkes · 2024-04-26T12:25:41Z

+
+Pointer<c.ObjCBlock> newClosureBlock(Pointer<Void> invoke, Function fn) =>
+    _newBlock(invoke, _registerBlockClosure(fn), _closureBlockDesc,
+        _blockClosureDisposer.sendPort.nativePort, (1 << 25));


document the flag

dcharkes · 2024-04-26T12:26:17Z

+        _blockClosureDisposer.sendPort.nativePort, (1 << 25));
+
+final _blockClosureRegistry = <int, Function>{};
+int _blockClosureRegistryLastId = 0;


nit newlines

dcharkes · 2024-04-26T12:42:32Z

+    throw Exception('Command failed: $exec ${args.join(" ")}');
+  }
+  print('Generated $output');
+}


Nit: Try to DynamicLibrary.open(...).lookup(...) as a sanity check?

dcharkes · 2024-04-26T12:45:31Z

 void main() {
  group('NSString', () {
+    setUpAll(() {
+      DynamicLibrary.open('test/objective_c.dylib');


Are we reyling on the fact that we load the symbols into the process this way? And then all the @Natives use process resolution?

I think that's fine on MacOS on iOS.
It doesn't work on Android/Linux. (See benchmarks/FfiCall/dart/dlopen_helper.dart in the SDK.)

Please docoment somewhere that we're doing this, with a todo to address for when we switch to native assets.

Added benefit, we don't have to change the API/FFIgen config when we switch to native assets.

Done. Added a note about this to #1068

dcharkes · 2024-04-26T12:51:46Z

  group('Automatic reference counting', () {
    setUpAll(() {
      logWarnings();
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');


This and all other places, leave a TODO that we remove this when switching to native assets.

dcharkes · 2024-04-26T12:55:24Z

 import 'dart:io';

 import 'package:ffi/ffi.dart';
+import 'package:objective_c/src/core.dart' as objc;


Consider making the import url 'package:objective_c/src/internal_for_testing.dart' so that it's self-documenting in tests where we use it.

Otherwise add a comment here and maybe use show to show the precise function.

dcharkes · 2024-04-26T12:55:49Z

      doGC();
+      await Future<void>.delayed(Duration.zero); // Let dispose message arrive.
      expect(lib.getBlockRetainCount(rawBlock), 0);
+      expect(objc.blockHasRegisteredClosure(rawBlock.cast()), false);


coveralls · 2024-04-28T23:22:17Z

coverage: 91.319% (+0.01%) from 91.309%
when pulling 37fc754 on blockdelete
into 042af36 on main.

liamappelbe added 3 commits April 22, 2024 15:02

ObjC example, and infra for shipping native code with plugin

ad8a6fe

Fix the block leak

3462a57

Mostly working

811c2f9

github-actions Bot added type-infra A repository infrastructure change or enhancement package:ffigen labels Apr 24, 2024

liamappelbe added the package:objective_c label Apr 24, 2024

liamappelbe added 4 commits April 24, 2024 15:33

Fix analysis

66188f2

Fix analysis

e856258

Fix example linker error

c7d4e07

fmt

a9ce06f

liamappelbe changed the title ~~[WIP] Delete the Dart functions associated with closure blocks~~ Delete the Dart functions associated with closure blocks Apr 25, 2024

liamappelbe marked this pull request as ready for review April 25, 2024 23:43

liamappelbe requested a review from dcharkes April 25, 2024 23:43

dcharkes approved these changes Apr 26, 2024

View reviewed changes

liamappelbe added 2 commits April 29, 2024 11:07

Daco's comments

4b1644e

Merge branch 'main' into blockdelete

15e246b

Fix NSData

37fc754

liamappelbe mentioned this pull request Apr 29, 2024

NSData.dataWithBytesNoCopy_length_ is crashing #1107

Closed

liamappelbe merged commit 6a9282c into main Apr 29, 2024

liamappelbe deleted the blockdelete branch April 29, 2024 00:53

liamappelbe mentioned this pull request Jun 11, 2024

Fix #835 #1203

Merged

Conversation

liamappelbe commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Health

Changelog Entry ✔️

Coverage ⚠️

License Headers ✔️

Package publish validation ✔️

Uh oh!

dcharkes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

liamappelbe commented Apr 24, 2024 •

edited

Loading

github-actions Bot commented Apr 24, 2024 •

edited

Loading

coveralls commented Apr 28, 2024 •

edited

Loading