Add ModuleDefinition.ImmediateRead #713

Merged
mrvoorhe merged 1 commit from master-immediate-read into master 2021-02-12 12:51:43 -06:00
mrvoorhe commented 2021-01-26 11:04:36 -06:00 (Migrated from github.com)

We are going to use this to do a multi stage load in parallel. We will be able to greatly reduce our load times using this new API. The way it's going to work is

Stage 1 - In parallel, load the assemblies with ReadingMode.Deferred.
Stage 2 - Populate our AssemblyResolver with a cache of all read assemblies.
Stage 3 - In parallel, call ImmediateRead.

What I really want is an API to load everything in stage 3. I found that ImmediateRead does not load method bodies. I don't know if/how you want want something like this exposed via the public API. For now I'm iterating the data model and forcing things to load that ImmediateRead did not cover.

We are going to use this to do a multi stage load in parallel. We will be able to greatly reduce our load times using this new API. The way it's going to work is Stage 1 - In parallel, load the assemblies with ReadingMode.Deferred. Stage 2 - Populate our AssemblyResolver with a cache of all read assemblies. Stage 3 - In parallel, call ImmediateRead. What I really want is an API to load everything in stage 3. I found that ImmediateRead does not load method bodies. I don't know if/how you want want something like this exposed via the public API. For now I'm iterating the data model and forcing things to load that ImmediateRead did not cover.
mrvoorhe (Migrated from github.com) reviewed 2021-01-26 11:06:37 -06:00
mrvoorhe (Migrated from github.com) commented 2021-01-26 11:06:37 -06:00

@jbevain You didn't include this in the changes you sent to me. I added this for 2 reasons.

  1. It seemed like the easiest way to write a test that meant something.

  2. I saw some other behaviors such as writing and symbol reading checked the reading mode. It seemed like it would be better if the reading mode was updated. However, I'm not certain this is correct. Does this seem sensible to you?

@jbevain You didn't include this in the changes you sent to me. I added this for 2 reasons. 1) It seemed like the easiest way to write a test that meant something. 2) I saw some other behaviors such as writing and symbol reading checked the reading mode. It seemed like it would be better if the reading mode was updated. However, I'm not certain this is correct. Does this seem sensible to you?
mrvoorhe commented 2021-02-04 07:33:25 -06:00 (Migrated from github.com)

@jbevain Can you take a look?

@jbevain Can you take a look?
mrvoorhe commented 2021-02-11 07:09:44 -06:00 (Migrated from github.com)

I updated the PR to more sensibly handle a module with no image. Now ImmediateRead will do nothing if there was no image. Previously it would throw.

I updated the PR to more sensibly handle a module with no image. Now ImmediateRead will do nothing if there was no image. Previously it would throw.
mrvoorhe commented 2021-02-11 07:10:56 -06:00 (Migrated from github.com)

@jbevain Does this seem like a change you'd be willing to take?

@jbevain Does this seem like a change you'd be willing to take?
jbevain (Migrated from github.com) reviewed 2021-02-11 17:37:58 -06:00
@ -283,6 +277,27 @@ namespace Mono.Cecil.Tests {
}
jbevain (Migrated from github.com) commented 2021-02-11 17:37:58 -06:00

Missing a space before the opening (

Missing a space before the opening (
jbevain (Migrated from github.com) reviewed 2021-02-11 17:39:38 -06:00
jbevain (Migrated from github.com) commented 2021-02-11 17:39:38 -06:00

I say just move this to ImmediateRead ()

I say just move this to `ImmediateRead ()`
jbevain commented 2021-02-11 17:40:02 -06:00 (Migrated from github.com)

Thanks Mike, please just address the two tidbits and that's good to go!

Thanks Mike, please just address the two tidbits and that's good to go!
mrvoorhe (Migrated from github.com) reviewed 2021-02-12 08:14:50 -06:00
@ -283,6 +277,27 @@ namespace Mono.Cecil.Tests {
}
mrvoorhe (Migrated from github.com) commented 2021-02-12 08:14:49 -06:00

Fixed

Fixed
mrvoorhe (Migrated from github.com) reviewed 2021-02-12 08:14:54 -06:00
mrvoorhe (Migrated from github.com) commented 2021-02-12 08:14:54 -06:00

Done

Done
jbevain (Migrated from github.com) approved these changes 2021-02-12 12:07:31 -06:00
jbevain commented 2021-02-12 12:51:48 -06:00 (Migrated from github.com)

Thanks Mike!

Thanks Mike!
Sign in to join this conversation.
No reviewers
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-jbevain/cecil!713
No description provided.