search

justinstenning / SharpDisasm

SharpDisasm - x86 / x86-64 disassembler for .NET
https://www.nuget.org/packages/SharpDisasm
Other
214 stars 39 forks source link

Use abstract byte code interface to remove unsafe/pointer code #3

Closed tgiphil closed 8 years ago

justinstenning commented 8 years ago

Thanks Phil, I need to do some performance tests first on the change from unsafe and use of assemblycodefrom memory.

tgiphil commented 8 years ago

Great! Let me now it turns out. I could change the interface to an abstract object to cut down a few more cycles.

I will be using this in another project that can't provide a byte array or pointer to the disassembler.

Btw. Awesome project!

tgiphil commented 8 years ago

I added a unit test that uses IntPtr. On my Intel Core i7-3770k @ 3.5 workstation takes 512ms to 495ms respectively for memory or byte array for 600,000 instructions covering 4,200,000 bytes.

justinstenning commented 8 years ago

I'm unable to check it right now but my gut is telling me there will be some significant reductions in performance, I think that perhaps it will be worth providing an IAssemblyCode class that supports the faster approach for those that can use it.

I'll try to test it out asap, however I am fairly busy so might not be for a week.

Thx glad you like it! I appreciate you taking the time to try and improve it!

tgiphil commented 8 years ago

I used the DisassembleLargeBuffer unit test as the benchmark and modified it to run an additional 10x iterations. I found no noticeable performance difference with the patch.

If possible, could you push out a new pre-release NuGet package with this patch? That way we can use the new functionality in another project and still use NuGet package system.

justinstenning commented 8 years ago

Thanks for the effort you have put in Phil. Performance tests appear to match up pretty good.

Couple of changes needed before I can complete the pull request if that is ok:

  1. can you please add an IndexOutOfRange check to the AssemblyCodeMemory this[]
  2. Can you please use the Tabs as 4 spaces setting in your IDE and resave those files
  3. Please change the filename comment at start of your new files to have correct filename
  4. Question: is there are reason you are not exposing the IAssemblyCode implementations as public? If there is no particular reason they might as well be exposed as public to ease use if anyone makes use of the classes outside of the disassembler.

Thanks again

tgiphil commented 8 years ago

Thanks for the feedback. I have incorporated it into this pull request.

Note: Regarding #4, the IAssemblyCode interface is public and the implementations are now internal as generally public interfaces should not expose internal implementation unless they should be consumed elsewhere.