search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

[breaking change] Remove Dart_UpdateExternalSize and Dart_UpdateFinalizableExternalSize from the Dart C API #52568

Open a-siva opened 1 year ago

a-siva commented 1 year ago

Change Intent

This breaking change proposes removing

Dart_UpdateExternalSize
Dart_UpdateFinalizableExternalSize

from the Dart C API as they are not being used by any of the embedders. Please see https://github.com/dart-lang/sdk/issues/42078 for more details and discussion in https://dart-review.googlesource.com/c/sdk/+/256025

Justification

Code cleanup (removing unused code from the sources).

Impact

We believe nobody uses this API and should not impact any users or embedders.

Mitigation

No mitigation required

a-siva commented 1 year ago

//cc @dcharkes @rmacnak-google @bkonyi

kevmoo commented 1 year ago

SGTM!

itsjustkevin commented 1 year ago

@a-siva could we approach this by deprecating the members in question for a cycle or two and announcing before fully removing them?

We may not have a complete understanding of who is utilizing the above APIs, this would give users time to migrate to another path.

dcharkes commented 1 year ago

I have no objections, but please ensure to bump the major version to 3 in runtime/include/dart_version.h.

This will create a breaking change for everyone using dart_api_dl.h and create a before and after. (Not only the people using these symbols.)

Justification Code cleanup

I'm not sure if this is worth the hassle of bumping the dart_api_dl.h version.

How much code can we clean up with this change?

The infrastructure itself for removing external size is used in native finalizers as well, so this is only about removing the symbols from the dart_api, correct?

@itsjustkevin these are symbols in C/C++, not in Dart.

cc @mkustermann, @mraleph for visibility

itsjustkevin commented 1 year ago

Thanks for clarification @dcharkes!

Plugging in @Hixie @vsmenon and @grouma for breaking change approval.

grouma commented 1 year ago

SGTM

Hixie commented 1 year ago

fine by me. (fyi @zanderso @dnfield)

dnfield commented 1 year ago

Sgtm

zanderso commented 1 year ago

sgtm

mkustermann commented 1 year ago

I have no objections, but please ensure to bump the major version to 3 in runtime/include/dart_version.h.

This will create a breaking change for everyone using dart_api_dl.h and create a before and after. (Not only the people using these symbols.)

That's a good point. There's a few options

IMHO we should choose a solution that doesn't break perfectly fine code.

dcharkes commented 1 year ago
  • only remove them from Dart C API and not from dart_api_dl.h, possibly making them NOPs

This option also crossed my mind, we would be lying in our semantic versioning then regarding these two specific symbols, but it might be the least worst option.

  • not increasing version number: only people actually using the symbols (which there may be none) are affected

That's a bad idea if we do actually remove the symbol from dart_api_dl.h, it would break binary compatibility. Did you mean something else?

IMHO we should choose a solution that doesn't break perfectly fine code.

++

bkonyi commented 1 year ago

Is there any reason we can't just mark these APIs as deprecated and make them no-ops until we're ready to do a larger cleanup?

mkustermann commented 1 year ago

not increasing version number: only people actually using the symbols (which there may be none) are affected

That's a bad idea if we do actually remove the symbol from dart_api_dl.h, it would break binary compatibility. Did you mean something else?

The way the dart_api_dl.h is implemented allows for missing symbols (those will simply be nullptr). Customer using dart_api_dl.h but not using those to-be-removed symbols will continue to work - even if we remove them. But changing major number is guaranteed to break break everyone using dart_api_dl.h (that is because the dart_api_dl.c fails to initialize if the major version isn't correct - which IMHO is something we shouldn't have done)

vsmenon commented 1 year ago

lgtm

itsjustkevin commented 1 year ago

@a-siva marking this breaking change as approved.

a-siva commented 1 year ago

https://dart-review.googlesource.com/c/sdk/+/314900

knopp commented 1 year ago

FWIW, this broke super_native_extensions. We we're not using it for anything important (all native objects are small enough), but we had it wired and have been calling it.

dnfield commented 1 year ago

FWIW, this broke super_native_extensions. We we're not using it for anything important (all native objects are small enough), but we had it wired and have been calling it.

In general, Flutter has found it more useful to use a dispose pattern for native objects than to try to report sizes to the Dart GC. Reporting native sizes may trigger too many useless GCs (because the GC feels pressure from some heavy native object), and relying on GC to clean up native memory may come too late (becuase the GC doesn't run immediately on every object free).

dnfield commented 1 year ago

Plus, even in the engine codebase there were multiple objects that were guessing or just otherwise providing fake values, in some cases with a comment like "this seems to run the GC often enough" :)

knopp commented 1 year ago

I'm not against removing it. My problem is that it fails at runtime. The implementation of Dart_InitializeApiDL changed. The name in DartApiEntry is different. If the application does not get rebuilt against new version of dart_api_dl.c (or is not even written in c) then it will just fail at runtime.

dcharkes commented 1 year ago

then it will just fail at runtime.

I believe we settled for changing the behavior of the symbol accessible via dart_api_dl.h to be a no-op as a way to prevent breaking users. Did we fail to accomplish that? How does your application fail at runtime?

knopp commented 1 year ago

If I understand it correctly, to use dart_api_dl.h, you need to include dart_api_dl.c in your library. (we have code that does similar thing written in Rust).

Dart_InitializeApiDL inside dart_api_dl.c will iterate over provided DartApiEntries and set the global symbols to corresponding function pointer from DartAPIEntry.

However the names in DartAPIEntry have changed. New version of dart_api_dl.c knows about it and will handle deprecated symbols, but the old version will not.

If a FFI library included the old version of dart_api_dl.c then the values for Dart_UpdateFinalizableExternalSize and Dart_UpdateExternalSize will be null.

dcharkes commented 1 year ago

Ah right, https://dart-review.googlesource.com/c/sdk/+/314900 should have changed the VM implementation to no-ops. Not the C file that users compile into their project. cc @a-siva