-
Notifications
You must be signed in to change notification settings - Fork 538
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
Return of the Strings #4487
Return of the Strings #4487
Conversation
cbcceaf
to
b76220b
Compare
Draft release notes Here's a candidate release note for this change. Feel free to edit as desired and add it to this pull request in
|
2fc87dc
to
bcb2822
Compare
public int ManagedIndex; | ||
} | ||
|
||
// Widths include the terminating nul character nor the padding! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should "nor" be "not" here? Or should it read "Widths include neither the terminating nul character nor the padding"?
src/monodroid/CMakeLists.txt
Outdated
endif() | ||
|
||
if(DEFINED HOST_BUILD_NAME) | ||
set(XA_LIBRARY_OUTPUT_DIRECTORY "${XA_LIB_TOP_DIR}/${HOST_BUILD_NAME}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct indentation. Looks like you have Emacs set to turn 4 spaces into tabs, instead of just always expanding tabs in this file.
src/monodroid/CMakeLists.txt
Outdated
if (ENABLE_NDK) | ||
# Only Android builds need to go in separate directories, desktop builds have the same ABI | ||
set_target_properties( | ||
xamarin-app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here about tab expansion.
@@ -289,6 +292,14 @@ endif() | |||
|
|||
set(XAMARIN_APP_STUB_SOURCES ${SOURCES_DIR}/application_dso_stub.cc) | |||
add_library(xamarin-app SHARED ${XAMARIN_APP_STUB_SOURCES}) | |||
if (ENABLE_NDK) | |||
# Only Android builds need to go in separate directories, desktop builds have the same ABI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't immediately see where/why the ABI differs, but why must the ABI differ in the first place? Is this strictly necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because Release
builds use [MVID][32-bit-int]
for map storage, mixed with [JavaName][32-bit-int]
and Debug
, with this PR, uses [TypeName][32-bit-int]
for indexing and stores the map differently. It also needs to fix up the pointers on load time in Debug+InstantRun
case. Also, Release
uses more public symbols in libxamarin-app.so
than Debug
(5 vs 1, respectively). Cramming all of this into a single structure would become confusing and messy, so I made the ABI differ. I could leave both Release
and Debug
structures and fields in all builds, but that's just... untidy.
bcb2822
to
d122dfb
Compare
Changes: mono/mono@fda7399...ecde086 Context: mono/mono#18127 Context: mono/mono#19377 Context: dotnet#4487 * mono/mono@ecde08600b2: [runtime] Allocate the memory for gshared gparams from image sets. (#19361) (#19419) * mono/mono@8fb93012925: [meta] Add mono_type_get_name_full to public API (#19415) * mono/mono@7bfb441fb4c: Fix Windows .msi build using newer xar * mono/mono@e77cea19105: Fix Windows .msi build with recent cygwin updates
Depends on: PR #4515 |
Changes: mono/mono@fda7399...ecde086 Context: mono/mono#18127 Context: mono/mono#19377 Context: #4487 * mono/mono@ecde08600b2: [runtime] Allocate the memory for gshared gparams from image sets. (#19361) (#19419) * mono/mono@8fb93012925: [meta] Add mono_type_get_name_full to public API (#19415) * mono/mono@7bfb441fb4c: Fix Windows .msi build using newer xar * mono/mono@e77cea19105: Fix Windows .msi build with recent cygwin updates
d122dfb
to
17f35b3
Compare
Changes: mono/mono@fda7399...ecde086 Context: mono/mono#18127 Context: mono/mono#19377 Context: #4487 * mono/mono@ecde08600b2: [runtime] Allocate the memory for gshared gparams from image sets. (#19361) (#19419) * mono/mono@8fb93012925: [meta] Add mono_type_get_name_full to public API (#19415) * mono/mono@7bfb441fb4c: Fix Windows .msi build using newer xar * mono/mono@e77cea19105: Fix Windows .msi build with recent cygwin updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release note approved!
3efbc83
to
d111b3e
Compare
WIP commit message:
|
bw.Write (moduleData.JavaNameWidth); | ||
bw.Write (moduleData.ManagedNameWidth); | ||
bw.Write (moduleName.Length); | ||
bw.Write (outputEncoding.GetBytes (moduleName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rando fear, but nothing requires that assembly names be ASCII, so if moduleName
contains non-ASCII characters, outputEncoding.GetBytes(moduleName).Length
will not equal moduleName.Length
.
This should probably instead be:
byte[] moduleNameBytes = outputEncoding.GetBytes (moduleName);
bw.Write (moduleNameBytes.Length);
bw.Write (moduleNameBytes);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, want me to update it here or in a new PR?
Fixes: dotnet#4415 Context: dotnet@ce2bc68 This commit partially reverts ce2bc68 in the sense that it restores the use of type names for Java-to-Managed and Managed-to-Java type lookups for **Debug** builds only. The reason for the change is that in order for the MVID and type token based lookups to work we need two things: - for the MVID to remain unchanged in each mapped assembly - for the type token to remain unchanged in each mapped assembly However, in incremental builds neither of the above requirements is met **unless** the application is fully rebuilt and, thus, the typemaps are regenerated from scratch, which obviously doesn't sit well with the incremental nature of incremental builds :) With MVID/token id maps, every change to any source code that's built into a mapped assembly, may cause the assembly to change its MVID and renaming of any type, removing or adding a type, will rearrange the type definition table in the resulting assembly, thus changing the type token ids (which are basically offsets into the type definition table in the PE executable). This is what may cause an app to crash on the runtime with an exception similar to: android.runtime.JavaProxyThrowable: System.NotSupportedException: Cannot create instance of type 'com.glmsoftware.OBDNowProto.SettingsFragmentCompat': no Java peer type found. at Java.Interop.JniPeerMembers+JniInstanceMethods..ctor (System.Type declaringType) [0x0004b] in <e3e4dfa992a7411b85acfe193481be3e>:0 at Java.Interop.JniPeerMembers+JniInstanceMethods.GetConstructorsForType (System.Type declaringType) [0x00031] in <e3e4dfa992a7411b85acfe193481be3e>:0 at Java.Interop.JniPeerMembers+JniInstanceMethods.StartCreateInstance (System.String constructorSignature, System.Type declaringType, Java.Interop.JniArgumentValue* parameters) [0x00038] in <e3e4dfa992a7411b85acfe193481be3e>:0 at AndroidX.Preference.PreferenceFragmentCompat..ctor () [0x00034] in <005e3ae6340747e1aea6d08b095cf286>:0 at com.glmsoftware.OBDNowProto.SettingsFragmentCompat..ctor () [0x00026] in <a8dbee4be1674aa08cce57b50f21e347>:0 at com.glmsoftware.OBDNowProto.SettingsActivity.OnCreate (Android.OS.Bundle bundle) [0x00083] in <a8dbee4be1674aa08cce57b50f21e347>:0 at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) [0x00011] in <c56099afccf04721853684f376a89527>:0 at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.3(intptr,intptr,intptr) at crc64596a13587a898911.SettingsActivity.n_onCreate(Native Method) at crc64596a13587a898911.SettingsActivity.onCreate(SettingsActivity.java:40) at android.app.Activity.performCreate(Activity.java:7825) at android.app.Activity.performCreate(Activity.java:7814) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1306) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3245) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3409) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:83) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016) at android.os.Handler.dispatchMessage(Handler.java:107) at android.os.Looper.loop(Looper.java:214) at android.app.ActivityThread.main(ActivityThread.java:7356) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930) Thus, we need to revert the Debug builds to the variation of the old string-based type name mapping. This commit implements it in a slightly leaner form than it was implemented before ce2bc68 landed in that only one copy of each set of type names (Java and managed) is maintained in both memory and on disk, at a tiny (sub millisecond) expense at the run time to fix up pointers between the two tables.
d111b3e
to
a3fd7f6
Compare
Fixes: #4415 Context: ce2bc68 Commit ce2bc68 optimized type mappings between managed types and Java types in large part by removing strings from the managed -> JNI mapping: instead of using an assembly-qualified *string* as a key, the assembly MVID & type metadata token were used as keys. This setup works reliably in Release apps, in which the assemblies don't change. In a commercial Debug with Fast Deployment situation, it falls down badly because every change to any source code that's built into a mapped assembly may cause the assembly to change its MVID, and renaming of any type -- removing or adding a type -- will rearrange the type definition table in the resulting assembly, thus changing the type token ids (which are basically offsets into the type definition table in the PE executable). This is what may cause an app to crash on the runtime with an exception similar to: android.runtime.JavaProxyThrowable: System.NotSupportedException: Cannot create instance of type 'com.glmsoftware.OBDNowProto.SettingsFragmentCompat': no Java peer type found. at Java.Interop.JniPeerMembers+JniInstanceMethods..ctor (System.Type declaringType) [0x0004b] in <e3e4dfa992a7411b85acfe193481be3e>:0 at Java.Interop.JniPeerMembers+JniInstanceMethods.GetConstructorsForType (System.Type declaringType) [0x00031] in <e3e4dfa992a7411b85acfe193481be3e>:0 at Java.Interop.JniPeerMembers+JniInstanceMethods.StartCreateInstance (System.String constructorSignature, System.Type declaringType, Java.Interop.JniArgumentValue* parameters) [0x00038] in <e3e4dfa992a7411b85acfe193481be3e>:0 at AndroidX.Preference.PreferenceFragmentCompat..ctor () [0x00034] in <005e3ae6340747e1aea6d08b095cf286>:0 at com.glmsoftware.OBDNowProto.SettingsFragmentCompat..ctor () [0x00026] in <a8dbee4be1674aa08cce57b50f21e347>:0 at com.glmsoftware.OBDNowProto.SettingsActivity.OnCreate (Android.OS.Bundle bundle) [0x00083] in <a8dbee4be1674aa08cce57b50f21e347>:0 at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_savedInstanceState) [0x00011] in <c56099afccf04721853684f376a89527>:0 at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.3(intptr,intptr,intptr) at crc64596a13587a898911.SettingsActivity.n_onCreate(Native Method) at crc64596a13587a898911.SettingsActivity.onCreate(SettingsActivity.java:40) at android.app.Activity.performCreate(Activity.java:7825) at android.app.Activity.performCreate(Activity.java:7814) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1306) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3245) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3409) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:83) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016) at android.os.Handler.dispatchMessage(Handler.java:107) at android.os.Looper.loop(Looper.java:214) at android.app.ActivityThread.main(ActivityThread.java:7356) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930) A "workaround" would be fully rebuild the application, which negates the point to incremental builds and the inner-dev-loop cycle. The fix is to partially revert ce2bc68 in the sense that it restores the use of string-based type names for Java-to-Managed and Managed-to-Java type lookups for ***Debug*** builds only. Unlike the pre-ce2bc689 world, only *one* copy of the set of string names is present within the data structures, at a tiny (sub millisecond) expense at the run time to fix up pointers between the two tables. ~~ File Formats ~~ All data in all file formats remains little-endian. In Debug configuration builds, each assembly will have a corresponding `*.typemap` file which will be loaded at runtime. The file format in pseudo-C++: struct DebugTypemapFileHeader { byte magic [4]; // "XATS" uint32_t format_version; // 2 uint32_t entry_count; uint32_t java_type_name_width; uint32_t managed_type_name_width; uint32_t assembly_name_size; byte assembly_name [assembly_name_size]; DebugTypemapFileJavaToManagedEntry java_to_managed [entry_count]; DebugTypemapFileManagedToJavaEntry managed_to_java [entry_count]; } struct DebugTypemapFileJavaToManagedEntry { byte jni_name [DebugTypemapFileHeader::java_type_name_width]; uint32_t managed_index; // Index into DebugTypemapFileHeader::managed_to_java }; struct DebugTypemapFileManagedToJavaEntry { byte managed_name [DebugTypemapFileHeader::java_type_name_width]; uint32_t jni_index; // Index into DebugTypemapFileHeader::java_to_managed }; `DebugTypemapFileHeader::java_type_name_width` and `DebugTypemapFileHeader::managed_type_name_width` are the maximum length + 1 (terminating NUL) for JNI names and assembly-qualified managed names. `DebugTypemapFileJavaToManagedEntry::jni_name` and `DebugTypemapFileManagedToJavaEntry::managed_name` are NUL-padded.
Context: dotnet#4487 7117414 raised the acceptable build times for three tests: * `Build_From_Clean_DontIncludeRestore` * `Build_AndroidManifest_Change` * `Build_JLO_Change` Unfortunately, we didn't increase the times enough. I raised them a bit more, so they should pass more reliably. In a future PR, we could consider using the ABI of the connected emulator/device to only build & generate an APK for that specific device. I could see this improving the regressed timings here, as the `_GenerateJavaStubs` and native assembly compilation are taking additional time per-ABI.
Context: https://build.azdo.io/3616517 Context: #4487 Commit 7117414 raised the acceptable build times for three tests: * `Build_From_Clean_DontIncludeRestore` * `Build_AndroidManifest_Change` * `Build_JLO_Change` Unfortunately, we didn't increase the times enough. I raised them a bit more, so they should pass more reliably. In a future PR, we could consider using the ABI of the connected emulator/device to only build & generate an APK for that specific device. I could see this improving the regressed timings here, as the `_GenerateJavaStubs` and native assembly compilation are taking additional time per-ABI.
Context: https://build.azdo.io/3616517 Context: #4487 Commit 7117414 raised the acceptable build times for three tests: * `Build_AndroidManifest_Change` * `Build_JLO_Change` Unfortunately, we didn't increase the times enough. I raised them a bit more, so they should pass more reliably. In a future PR, we could consider using the ABI of the connected emulator/device to only build & generate an APK for that specific device. I could see this improving the regressed timings here, as the `_GenerateJavaStubs` and native assembly compilation are taking additional time per-ABI.
Context: https://build.azdo.io/3616517 Context: #4487 Commit 7117414 raised the acceptable build times for three tests: * `Build_From_Clean_DontIncludeRestore` * `Build_AndroidManifest_Change` * `Build_JLO_Change` Unfortunately, we didn't increase the times enough. I raised them a bit more, so they should pass more reliably. In a future PR, we could consider using the ABI of the connected emulator/device to only build & generate an APK for that specific device. I could see this improving the regressed timings here, as the `_GenerateJavaStubs` and native assembly compilation are taking additional time per-ABI.
Fixes: #4415
Context: ce2bc68
This commit partially reverts ce2bc68
in the sense that it restores the use of type names for Java-to-Managed
and Managed-to-Java type lookups for Debug builds only.
The reason for the change is that in order for the MVID and type token
based lookups to work we need two things:
However, in incremental builds neither of the above requirements is
met unless the application is fully rebuilt and, thus, the typemaps
are regenerated from scratch, which obviously doesn't sit well with the
incremental nature of incremental builds :)
With MVID/token id maps, every change to any source code that's built
into a mapped assembly, may cause the assembly to change its MVID and
renaming of any type, removing or adding a type, will rearrange the type
definition table in the resulting assembly, thus changing the type token
ids (which are basically offsets into the type definition table in the
PE executable). This is what may cause an app to crash on the runtime
with an exception similar to:
Thus, we need to revert the Debug builds to the variation of the old
string-based type name mapping. This commit implements it in a slightly
leaner form than it was implemented before
ce2bc68 landed in that only one copy of
each set of type names (Java and managed) is maintained in both memory
and on disk, at a tiny (sub millisecond) expense at the run time to fix
up pointers between the two tables.