-
-
Notifications
You must be signed in to change notification settings - Fork 624
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
Add ModuleDefinition.ImmediateRead #713
Add ModuleDefinition.ImmediateRead #713
Conversation
Mono.Cecil/AssemblyReader.cs
Outdated
@@ -166,6 +166,7 @@ protected override void ReadModule () | |||
|
|||
public void ReadModule (ModuleDefinition module, bool resolve_attributes) | |||
{ | |||
module.ReadingMode = ReadingMode.Immediate; |
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.
@jbevain You didn't include this in the changes you sent to me. I added this for 2 reasons.
-
It seemed like the easiest way to write a test that meant something.
-
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?
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 say just move this to ImmediateRead ()
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.
Done
@jbevain Can you take a look? |
1da28d1
to
219371c
Compare
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. |
@jbevain Does this seem like a change you'd be willing to take? |
Test/Mono.Cecil.Tests/ModuleTests.cs
Outdated
{ | ||
using (var module = GetResourceModule ("hello.exe", ReadingMode.Deferred)) { | ||
Assert.AreEqual (ReadingMode.Deferred, module.ReadingMode); | ||
module.ImmediateRead(); |
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.
Missing a space before the opening (
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.
Fixed
Thanks Mike, please just address the two tidbits and that's good to go! |
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.
219371c
to
2ad9ff8
Compare
Thanks Mike! |
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.