makeelf icon indicating copy to clipboard operation
makeelf copied to clipboard

Replace bytes_xor with much faster numpy based version

Open reticulatedpines opened this issue 4 years ago • 3 comments

Hi, I was using your code on some fairly large files and writing out the ELF was quite slow. Most of this was in bytes_xor(), so I found a faster implementation using numpy.

If you don't want the numpy dependency, that's perfectly understandable. Perhaps this could be refactored so it uses the fast version if numpy is already available.

I did very little testing - just checking that for the files I was working with, SHA1 was the same with old vs fast implementation of bytes_xor().

reticulatedpines avatar Feb 15 '21 15:02 reticulatedpines

For me, adding dependency, like numpy is acceptable. This is fairly common library, so should not hurt. But before merging this, definitely dependencies should be fixed. As you can see, at the moment tests are failing because of that.

v3l0c1r4pt0r avatar Feb 16 '21 17:02 v3l0c1r4pt0r

Yes, sorry about the broken build. I didn't know what tests, if any, would run. I don't know the process you want to follow for your repo, and I'm unfamiliar with the Github interface. What's the best approach? Make changes in Github? Make changes locally and then a new PR?

reticulatedpines avatar Feb 16 '21 21:02 reticulatedpines

Don't worry about the builds. They are here to help you. You can push to this PR as many times as you like. So,please do not create another PR. To push updates to this PR simply push to your branch again. Regarding the problem, tests are just to avoid errors and bugs. I'm pushing new versions to PyPI, from time to time. To not break that, all dependencies has to be specified explicitly. Otherwise, once someone installs makeelf on fresh system, he would have it broken due to a lack of numpy. That's what definitely has to be fixed. To be honest, I didn't add any dependencies in the past, so if you still want to continue the topic, you have to do some research on your own. But, I don't believe it is a rocket science :)

v3l0c1r4pt0r avatar Feb 21 '21 11:02 v3l0c1r4pt0r