Skip to content
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

Jump to bottom

gh-88339: enable fast seeking of uncompressed unencrypted zipfile.ZipExtFile #27737

Merged
merged 7 commits into from Aug 6, 2022
Merged

gh-88339: enable fast seeking of uncompressed unencrypted zipfile.ZipExtFile #27737

merged 7 commits into from Aug 6, 2022
+22 −5

Conversation

JuniorJPDJ
Copy link
Contributor

@JuniorJPDJ JuniorJPDJ commented Aug 12, 2021 •
edited by bedevere-bot

At the moment stored ZipExtFile is being read to the place of seek like all other compressed variants.
It's not needed as it's possible to freely seek uncompressed file inside zip without this penalty.

Lots of apps depend on ZipExtFile seeking ability and this patch would increase performance and lower IO penalty significantly.

It disables CRC checking after first seek as it's impossible to check CRC if we are not reading whole file.

I've been using patched zipfile for almost a year now and I can say it works good ;)

https://bugs.python.org/issue44173 / #88339

redo of #26227 after a little fail

@JuniorJPDJ JuniorJPDJ mentioned this pull request Aug 12, 2021
Closed
@gpshead gpshead self-assigned this Aug 18, 2021
gpshead
gpshead requested changes Aug 18, 2021
View changes
Copy link
Member

@gpshead gpshead left a comment

Does the test_zipfile.py unittest explicitly cover this case of seeking within a ZIP_STORED entry? If so, mention that and drop a link to where in a comment here.

Lib/zipfile.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 18, 2021

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from bc54dce to 3c76400 Compare Aug 18, 2021
gpshead
gpshead requested changes Aug 19, 2021
View changes
Copy link
Member

@gpshead gpshead left a comment

How about adding an explicit seek + read test case to Lib/test/test_zipfile.py's StoredTestsWithSourceFile class that checks that the data read matches what is expected at the seek'ed to offset?

@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Aug 19, 2021 •
edited

I don't get why this test fails. Do you maybe have any idea?
EDIT: fixed

@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Aug 19, 2021

How about adding an explicit seek + read test case to Lib/test/test_zipfile.py's StoredTestsWithSourceFile class that checks that the data read matches what is expected at the seek'ed to offset?

Will try to do, thanks!

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 25, 2021

There is a unittest explicitly covering this case. The one which fails.

@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from 3c76400 to 518d886 Compare Aug 26, 2021
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Aug 26, 2021 •
edited

There is a unittest explicitly covering this case. The one which fails.

You are right! It's that unit test. I added one assert to it.

There were problems with read buffer which I fixed now. I rebased and added fixup! commit without amending previous one to make review easier. Just please, squash them when merging.

Please run CI and tell me if I'm still missing something. ;)

BTW. Thanks for your work guys!

@JuniorJPDJ JuniorJPDJ requested a review from gpshead Aug 26, 2021
@JuniorJPDJ JuniorJPDJ changed the title bpo-44173: better approach for seeking in non-compressed ZipExtFile bpo-44173: [zipfile] better approach for seeking in non-compressed ZipExtFile Aug 26, 2021
@JuniorJPDJ JuniorJPDJ changed the title bpo-44173: [zipfile] better approach for seeking in non-compressed ZipExtFile bpo-44173: enable fast seeking of uncompressed unencrypted zipfile.ZipExtFile Aug 27, 2021
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Aug 27, 2021 •
edited

Previous tests passed (some asyncio on windows x64 failed, but it's probably not my fault).
I just rebased and added news entry - from my side it's ready to merge. Am I missing something?

@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from 750d8ec to 6e4a3c7 Compare Aug 27, 2021
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Sep 19, 2021 •
edited

Can I ask for review please?

EDIT: Just rebased my branch.

@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from 6e4a3c7 to 0c4a434 Compare Sep 19, 2021
@JuniorJPDJ JuniorJPDJ changed the title bpo-44173: enable fast seeking of uncompressed unencrypted zipfile.ZipExtFile gh-88339: enable fast seeking of uncompressed unencrypted zipfile.ZipExtFile Apr 17, 2022
@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from 0c4a434 to 293792e Compare Apr 17, 2022
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Apr 19, 2022

I have made the requested changes; please review again

(test is already here, I added some tell asserts)

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 19, 2022

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

gpshead
gpshead reviewed Apr 20, 2022
View changes
Lib/zipfile.py Outdated Show resolved Hide resolved
@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from c9417aa to e891ae4 Compare Apr 22, 2022
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Apr 30, 2022

Oh and I again forgot to tag you guys: @gpshead, @serhiy-storchaka

@JuniorJPDJ JuniorJPDJ mentioned this pull request Apr 10, 2022
Closed
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented May 22, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 22, 2022

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

@JuniorJPDJ JuniorJPDJ requested a review from serhiy-storchaka Jun 2, 2022
@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from 8f28620 to cf56601 Compare Jun 5, 2022
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Jun 15, 2022

Guys, what's wrong. It's over a year here now, I fixed everything you mentioned. Can I do something more to make this faster?

gpshead
gpshead approved these changes Jun 15, 2022
View changes
Copy link
Member

@gpshead gpshead left a comment

I feel like this could use more explicit testing around all possible seek edge cases, but that is true in general of the zipfile module and not specific to this PR. It appears to do the right thing and the existing tests that hopefully represent the more likely usages continue to pass.

@gpshead
Copy link
Member

@gpshead gpshead commented Jun 15, 2022

If I haven't merged this within a few days, please ping me again. I'm giving Serhiy some time to respond if desired.

@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Jun 22, 2022 •
edited

If I haven't merged this within a few days, please ping me again.

@gpshead ;)

Also I've a question - merging it now to main branch means this changes will be available from python 3.12, not 3.11?

@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Jun 26, 2022

If I haven't merged this within a few days, please ping me again.

@gpshead ;)

@JuniorJPDJ JuniorJPDJ force-pushed the JuniorJPDJ/zipfile-store-seek branch from cf56601 to 420ae62 Compare Jun 26, 2022
@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Jul 8, 2022

ping @gpshead

@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Jul 23, 2022

ping @gpshead

@JuniorJPDJ
Copy link
Contributor Author

@JuniorJPDJ JuniorJPDJ commented Aug 6, 2022

bump @gpshead

@gpshead gpshead merged commit 330f1d5 into python:main Aug 6, 2022
14 checks passed
@gpshead
Copy link
Member

@gpshead gpshead commented Aug 6, 2022

yep, this is in 3.12. the feature freeze window for a release closes upon the first beta.

@gpshead
Copy link
Member

@gpshead gpshead commented Aug 6, 2022

thanks for the improvement!

@JuniorJPDJ JuniorJPDJ deleted the JuniorJPDJ/zipfile-store-seek branch Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead

@serhiy-storchaka serhiy-storchaka

Assignees

@gpshead gpshead

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JuniorJPDJ @bedevere-bot @serhiy-storchaka @gpshead @the-knights-who-say-ni @ezio-melotti