[C++-ification] Introducing EmbeddedAssemblies class by grendello · Pull Request #2606 · dotnet/android

grendello · 2019-01-10T21:27:01Z

removed a number of MONO_API functions, they don't appear to be used anywhere (were supposed to be used by Mono Unreal Engine, but we're not sure if that's the case). We can restore them if needed
Simplified a lot of code
Removed printf family calls

jonpryor · 2019-01-11T18:59:43Z

src/monodroid/jni/embedded-assemblies.cc

+		int                       entry_length;
+		int                       value_offset;
+		const   char             *mapping;
+		struct  TypeMappingInfo  *next;


Yet another struct StructType which could instead be StructType ;-)

src/monodroid/jni/embedded-assemblies.cc

jonpryor · 2019-01-11T19:04:54Z

src/monodroid/jni/embedded-assemblies.cc

-}
-
-MONO_API int
-monodroid_embedded_assemblies_set_assemblies_prefix (const char *prefix)


There is Another Repo that cares about our "public API" surface: https://github.com/mono/Embeddinator-4000

In particular, they call monodroid_embedded_assemblies_set_assemblies_prefix():

https://github.com/mono/Embeddinator-4000/blob/c237e21b63df40799ef251350ee961846c549553/support/mono_embeddinator.c#L62

As such:

This export shouldn't be removed, and

I don't think EmbeddedAssemblies::assemblies_prefix can actually be a constexpr.

is it just this API or all of the ones which were here before?

it can - it's the default, we can use it as long as the 3rd party sets a new one, in which case we'll use the new value

I suppose I don't understand constexpr then; I thought constexpr was for compile-time constants.

@jonathanpeppers: Do you have a feel for which parts of our libmonodroid.so API Embeddinator-4000 uses?

@grendello: From what I can easily tell, monodroid_embedded_assemblies_set_assemblies_prefix is the only one:

$ git grep monodroid_ support/mono_embeddinator.c:MONO_API int monodroid_embedded_assemblies_set_assemblies_prefix (const char *prefix); support/mono_embeddinator.c: monodroid_embedded_assemblies_set_assemblies_prefix("assets/assemblies/");

Looking at git grep mono...they use mono.android.jar?!

/me backs away slowly and awaits @jonathanpeppers response.

@jonpryor you understand constexpr correctly, but see the updated code here. We should optimize for the most common case - and that is our prefix in this instance.

monodroid_embedded_assemblies_set_assemblies_prefix is the only one called by Embeddinator.

I was not able to put assembly files in /assemblies, because AAR files couldn't have arbitrary directories like that. I put them in assets/assemblies and called monodroid_embedded_assemblies_set_assemblies_prefix. I had to include instructions to leave .dll files uncompressed.

jonpryor · 2019-01-11T19:06:03Z

src/monodroid/jni/embedded-assemblies.cc

-	size_t si;
-
-	MonoAssembly *a = nullptr;
+	const char *culture = reinterpret_cast<const char*> (monoFunctions.assembly_name_get_culture (aname));


I suppose I should have asked this ages ago, but why monoFunctions and not mono?

Why is this reinterpret_cast<const char*> needed in the first place? DylibMono::assembly_name_get_culture() already returns const char*. This cast should not be needed.

because it's not Mono, just its functions :)

jonpryor · 2019-01-11T19:10:00Z

src/monodroid/jni/embedded-assemblies.cc

-
-	MonoAssembly *a = nullptr;
+	const char *culture = reinterpret_cast<const char*> (monoFunctions.assembly_name_get_culture (aname));
+	const char *asmname = reinterpret_cast<const char*> (monoFunctions.assembly_name_get_name (aname));


Ditto here:

const char* DylibMono::assembly_name_get_name (MonoAssemblyName *aname)

DylibMono::assembly_name_get_name() already returns a const char*. Why is there a cast?

jonpryor · 2019-01-11T19:14:51Z

src/monodroid/jni/embedded-assemblies.cc


-MONO_API const char *
-monodroid_typemap_managed_to_java (const char *managed)
+inline const char*


You're inlining a method with a for loop?!

Why?

To make it faster without incurring another call overhead? This way we end up with code identical, performance and size wise, as before.

jonpryor · 2019-01-11T19:15:34Z

src/monodroid/jni/embedded-assemblies.cc

-	const char              *data     = addr;
-
-	if (!p)
+	struct TypeMappingInfo *p = new TypeMappingInfo (); // calloc (1, sizeof (struct TypeMappingInfo));


Yet another struct that can be removed.

jonpryor · 2019-01-11T19:25:57Z

src/monodroid/jni/embedded-assemblies.cc

-
-	if (!p)
+	TypeMappingInfo *p = new TypeMappingInfo (); // calloc (1, sizeof (struct TypeMappingInfo));
+	if (p == nullptr)


This is now redundant/unnecessary/contradictory/a source of confusion; as per this and related comments:
#2515 (comment)

We have explicitly stated that new T will either return memory, or abort the process. There is thus no need to continue checking for nullptr return values, unless new(nothrow) T is used.

Thus, a philosophical question: should we have "error handling code" for scenarios that we know and guarantee cannot possibly happen? p will NEVER be nullptr; as such, this and the following line are dead.

Worse than dead, though; misleading: their mere presence suggests that perhaps new T can return NULL, which will be confusing to any newcomers to the codebase: some places are checking the return value, some don't, so which is correct? ¯_(ツ)_/¯

...which is yet another thing I missed/overlooked in PR #2599: it also contains places which use new, then check the return value.

We should be consistent.

Furthermore, consistency should be this:

If new T is used, do not check the return value. It cannot be NULL.

If new(nothrow) T is used, check the return value; it may be NULL.

jonpryor · 2019-01-11T19:34:18Z

src/monodroid/jni/embedded-assemblies.cc

-	should_register       = r;
-	should_register_data  = d;
+	if (assemblies_prefix_override != nullptr)
+		delete[] assemblies_prefix_override;


This should be free(assemblies_prefix_override), not delete[], as we use strdup().

This is in preparation for the changes in this PR , it was deliberate

* removed a number of `MONO_API` functions, they don't appear to be used anywhere (were supposed to be used by Mono Unreal Engine, but we're not sure if that's the case). We can restore them if needed * Simplified a lot of code * Removed `printf` family calls

grendello added the do-not-merge PR should not be merged. label Jan 10, 2019

grendello requested a review from jonpryor as a code owner January 10, 2019 21:27

grendello force-pushed the cppify+1 branch from 2598bf5 to f2bea65 Compare January 11, 2019 17:10

grendello changed the title ~~[WIP, C++-ification] Introducing EmbeddedAssemblies class~~ [C++-ification] Introducing EmbeddedAssemblies class Jan 11, 2019

grendello force-pushed the cppify+1 branch from f2bea65 to 7c7c86d Compare January 11, 2019 17:14

grendello added full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) and removed do-not-merge PR should not be merged. labels Jan 11, 2019

grendello mentioned this pull request Jan 11, 2019

[C++ification] Glue and utilities code changes #2611

Merged

jonpryor reviewed Jan 11, 2019

View reviewed changes

src/monodroid/jni/embedded-assemblies.cc Show resolved Hide resolved

jonpryor reviewed Jan 11, 2019

View reviewed changes

grendello force-pushed the cppify+1 branch from 7c7c86d to fed721a Compare January 11, 2019 19:21

jonpryor reviewed Jan 11, 2019

View reviewed changes

grendello force-pushed the cppify+1 branch from fed721a to f4f7cf9 Compare January 11, 2019 19:30

jonpryor reviewed Jan 11, 2019

View reviewed changes

grendello force-pushed the cppify+1 branch from f4f7cf9 to 8ecfaca Compare January 14, 2019 08:13

jonpryor merged commit 434d2d8 into dotnet:master Jan 14, 2019

grendello deleted the cppify+1 branch January 15, 2019 21:07

github-actions bot locked and limited conversation to collaborators Feb 1, 2024

Conversation

grendello commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

grendello Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

grendello Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grendello commented Jan 10, 2019 •

edited

Loading

grendello Jan 11, 2019 •

edited

Loading

grendello Jan 11, 2019 •

edited

Loading