Skip to content
This repository was archived by the owner on Aug 27, 2026. It is now read-only.

[Java.Interop.Tools.JavaCallableWrappers] Use ReadingMode.Deferred - #137

Merged
radekdoulik merged 1 commit into
dotnet:masterfrom
jonpryor:jonp-no-read-immediate
Mar 28, 2017
Merged

[Java.Interop.Tools.JavaCallableWrappers] Use ReadingMode.Deferred#137
radekdoulik merged 1 commit into
dotnet:masterfrom
jonpryor:jonp-no-read-immediate

Conversation

@jonpryor

Copy link
Copy Markdown
Contributor

A "funny" 1 thing happened when trying to
update xamarin-android to use a newer Java.Interop: while all the
tests passed, the macOS+xbuild PR builder -- which actually runs the
unit tests -- was really slow: 2 hr 26 minutes to run, vs. a
more normal 53 minutes.

Ouch.

@radekdoulik tried to verify that it wasn't
c794fab; that build took 1 hr 42 min.

Something between cecil/d0cb2b7 and cecil/0d05ab1 caused this, but
we don't know what, and even if we knew, that might not matter.

That said, we have found two apparent factors in the slowdown:

  1. Using ReaderParameters.ReadingMode to ReadingMode.Immediate
    (084e9f7), and
  2. Setting ReaderParameters.ReadSymbols to true.

There also appears to be something "wrong" with the .pdb files that
Roslyn on macOS is generating, in that the .pdb file appears to be
invalid; no real details at this time.

To verify:

$ csharp -r:bin/Debug/Xamarin.Android.Cecil.dll
csharp> using Mono.Cecil;
csharp> var rp = new ReaderParameters (ReadingMode.Immediate) { ReadSymbols = true };
csharp> var ad = AssemblyDefinition.ReadAssembly ("path/to/Mono.Android.dll", rp);

This "hangs", taking roughly 2 minutes to complete. (Oof.)

Change things to remove ReadingMode.Immediate, e.g.

csharp> var rp = new ReaderParameters () { ReadSymbols = true };

and AssemblyDefinition.ReadAssembly() is instantaneous.

So...why are we using ReadingMode.Immediate?

Hysterical raisens, mostly: as commit 084e9f7 noted, in the "previous
world order" Cecil loaded everything in memory, so it seemed perfectly
reasonable that to fix various file sharing bugs we could/should
continue doing the same thing.

That turned out to be...not quite right; see 40b75e9, in which the
DirectoryAssemblyResolver instance had to be turned into a member
instead of a constructor local parameter, because code "down the call
stack" was implicitly using the DirectoryAssemblyResolver instance.
This means we don't really need ReadingMode.Immediate at all,
and thus removing it should (1) not break anything, and (2) provide us
with a very nice performance improvement.

Footnotes

  1. Everything is always "funny"...

A "funny" [^0] thing happened when trying to
[update xamarin-android to use a newer Java.Interop][0]: while all the
tests passed, the macOS+xbuild PR builder -- which actually runs the
unit tests -- was *really slow*: [2 hr 26 minutes to run][1], vs. a
more [normal 53 minutes][2].

Ouch.

@radekdoulik tried to verify that it wasn't
c794fab; *that* build took [1 hr 42 min][3].

*Something* between cecil/d0cb2b7 and cecil/0d05ab1 caused this, but
we don't know what, and even if we knew, that might not matter.

That said, we have found two apparent factors in the slowdown:

1. Using `ReaderParameters.ReadingMode` to `ReadingMode.Immediate`
    (084e9f7), and
2. Setting `ReaderParameters.ReadSymbols` to `true`.

There also appears to be something "wrong" with the `.pdb` files that
Roslyn on macOS is generating, in that the `.pdb` file appears to be
invalid; no real details at this time.

To verify:

	$ csharp -r:bin/Debug/Xamarin.Android.Cecil.dll
	csharp> using Mono.Cecil;
	csharp> var rp = new ReaderParameters (ReadingMode.Immediate) { ReadSymbols = true };
	csharp> var ad = AssemblyDefinition.ReadAssembly ("path/to/Mono.Android.dll", rp);

This "hangs", taking roughly 2 minutes to complete. (Oof.)

Change things to *remove* `ReadingMode.Immediate`, e.g.

	csharp> var rp = new ReaderParameters () { ReadSymbols = true };

and `AssemblyDefinition.ReadAssembly()` is instantaneous.

So...why are we using `ReadingMode.Immediate`?

Hysterical raisens, mostly: as commit 084e9f7 noted, in the "previous
world order" Cecil loaded everything in memory, so it seemed perfectly
reasonable that to fix various file sharing bugs we could/should
continue doing the same thing.

That turned out to be...not quite right; see 40b75e9, in which the
`DirectoryAssemblyResolver` instance had to be turned into a member
instead of a constructor local parameter, because code "down the call
stack" was implicitly using the `DirectoryAssemblyResolver` instance.
This means we don't *really* need `ReadingMode.Immediate` *at all*,
and thus removing it should (1) not break anything, and (2) provide us
with a very nice performance improvement.

[0]: dotnet/android#532
[1]: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/775/
[2]: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/769/
[3]: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/776/

[^0]: Everything is always "funny"...
@radekdoulik
radekdoulik merged commit cd2e96b into dotnet:master Mar 28, 2017
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants