Closed xiaogdgenuine closed 4 years ago
The testing result in my 2015 Mac Book Pro:
100 files - old(0.107s):
100 files - new(0.0369s):
1k files - old(7.55s):
1k files - new(0.37s):
2k files - old(30.3s):
2k files - new(0.753s):
Thanks for sending this through! I'm reviewing, and will come back to you with feedback.
I wanted to let you know I haven’t forgotten about this. I’ve just been tied up with a couple of other things and will try to take a look in the next week or two.
No problem @abbeycode just take your times~ I have been using this PR in my project for a while, so far everything works :P.
Meanwhile, pls keep yourself safe out there 🤗 , it's a hard time for all of us.
So, I've had a chance to take a look, and I'd like to start with some high-level feedback before digging into a detailed review.
First, thanks! Those are some amazing performance improvements, and I'm excited to get them into the shipping library. These are some changes I'd like to see:
URKFileInfo
instance, but it can look up the URKFileInfo
instance that corresponds to the filename given. We'll need copies of the unit tests for the old method that instead call the new one, to make sure it behaves properlyURKFileInfo
instance the same way as extractBufferedData
v2.10
branch, not master
. I'd like to include it in that releaseunrar
library as read-only code. I need to be able to update that with upstream changes, without having to merge each time. I'll be there's a way to seek within the archive using the methods already in that API. I can help you figure it out if you'd likeHey, it's been a while since I heard from you on this. Are you interested in working on the changes I suggested, or should I make them myself?
Hi @abbeycode , I'm really really really... x100 sorry for this belated reply.
I have been allocated to a damn busy project for the last half year, and also need to be prepare for my adult education college entrance examination, so u know....got to make a living 😢 .
The main purpose I submit this PR is to show u the performance differences rather than merge it into the repo, so your suggestions about the PR is really helpful, I should know better about github contribution flow.
Sure I'm interested in working on your suggested changes, my examination will start next weekend and after that I shall have plenty of time to working on this, but if u are in hustle to match your release plan, pls feel free to help me haha :)
Another thing is that I found my new method break the Encrypted RAR archive, invalid data got exctracted by this method, I don't know why, I guess Encrypted RAR archive has a different way to iterating file headers, I will take time to investgate this later.
Hey, first of all, no problem at all on the delay - the world's turned upside down, and your contributions here are unpaid :)
I'm not going to hold up the release for this. I'll probably release v2.10 soon, and put this into v2.11. Take your time, and let me know if you need a hand debugging the encrypted archives issue.
Good luck with your exam!
@abbeycode I open another PR to show you my latest changes, I will close this one.
https://github.com/abbeycode/UnrarKit/pull/92
Also, a little note about my changes impacts the password-protected archive: It's a false alarm :).
I accidently initial all my rar archive with empty string password (password:@"" instead password:nil), in this case, if an archive is password protected, the "isPasswordProtected" method will return false, so my code-flow got messup, it's trying to extract an password protected file without password...
With this enhancement, we can now quickly locate an entry's position in the archive file, don't need to iterate headers and keep seeking the file until we locate the entry's position, now only one
seek
operation is required(For single volume archive)What I did is:
relativeOffsetToHeader
volNumber
attributeextractBufferedDataByOffsetOf
API to extract buffer data by offset, which will call a newRARSeekTo
API inunrar
to seek at the correct position of the archive file.And as you suggested, I add a Unit test file to compare performance difference after this enhancement, and since some test cases with the old way are quite slow, I comment them out in the test code, u can enable them if u want to test.
Pls forget my code style, I'm really not good on Objective-C and C++, I'm mainly a web guy and playing JS, this is probably the first time I touch some code that's so low level :P