classification
Title: zipimport is a bit slow
Type: performance Stage: resolved
Components: Interpreter Core Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Goplat, brett.cannon, catalin.iacob, eric.snow, python-dev, rhettinger, serhiy.storchaka, ysj.ray
Priority: normal Keywords: patch

Created on 2010-05-18 06:08 by Goplat, last changed 2013-02-16 21:54 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
zipimport_speedup.patch Goplat, 2010-05-18 06:08 patch to read .zip's central directory sequentially
zipimport_speedup-v2.patch catalin.iacob, 2012-06-04 21:55
attempt-fseek-seek_cur.patch catalin.iacob, 2012-11-25 15:41
zipimport-speedup-v3.patch catalin.iacob, 2012-11-25 15:41
zipimport-speedup-v4.patch catalin.iacob, 2013-01-06 21:05
zipimport-speedup-v4.patch serhiy.storchaka, 2013-01-07 15:01 Converted from UTF-16 CRLF review
Messages (14)
msg105954 - (view) Author: (Goplat) Date: 2010-05-18 06:08
Reading the list of files in a .zip takes a while because several seeks are done for each entry, which (on Windows at least) flushes stdio's buffer and forces a system call on the next read. For large .zips the effect on startup speed is noticeable, being perhaps 50ms per thousand files. Changing the read_directory function to read the central directory entirely sequentially would cut this time by more than half.
msg105960 - (view) Author: ysj.ray (ysj.ray) Date: 2010-05-18 10:09
When I perform some test on debian-5.0, I see the timing results almost the same before and after apply your patch(I modified the patch to against the trunk). 

Could you give some test result on windows? I can't see the speedups on debian-5.0.
msg106191 - (view) Author: (Goplat) Date: 2010-05-20 21:09
Zipping up the Lib directory from the python source (1735 files) as a test, it took on average 0.10 sec to read the zip before, 0.04 sec after.

(To test the time taken by zipimport isolated from other startup costs, instead of actually getting the zip in the import path, I just ran

import time, zipimport; start = time.clock(); 
zipimport.zipimporter("lib.zip"); print time.clock() - start)
msg162300 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2012-06-04 21:55
I updated Goplat's patch to the default branch.

It now needs to read 4 dummy bytes instead of 6 since an extra PyMarshal_ReadShortFromFile was added to the default branch in the mean time. I added an explicit dummy buffer instead of reading the dummy bytes into name (for cleanness and because name would overflow on hypothetical platforms where MAXPATHLEN + 5 < 8). Also added tests for the loop that skips the rest of the header by creating some zips with file comments; without the extra test, commenting out the loop didn't fail test_zipimport.

Running Goplat's test in msg106191 on Windows I get 0.032 sec before and 0.015 sec after. On Linux I see no significant difference.

AFAIK Mercurial (for example) ships with a zipped stdlib on Windows and they care quite a lot about startup time. Can this make it into 3.3?
msg174934 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-05 19:40
I suggest to use fseek(fp, relative_offset, SEEK_CUR).  It doesn't force a system call (at least on Linux) and may be a little faster than fread() or multiple getc().
msg176367 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2012-11-25 15:41
I tried Serhiy's suggestion in msg174934, see attached attempt-fseek-seek_cur.patch updated to current default. It makes no difference relative to the default branch for my test stdlib.zip, dummy reads still work better on Windows.

This makes sense if you follow the CRT's implementation, fseek calls _fseek_nolock which always calls _flush, regardless whether SEEK_CUR is used or not. This is the case with both VS2008 and VS2010. So this patch is a workaround for poor fseek performance in Microsoft's CRT, it doesn't cause performance issues on Linux but saves quite some milliseconds of startup time so I think it's worth the tradeoff.

I'll also upload zipimport_speedup-v3.patch updated to apply to current default and with error handling for failing freads since the fseeks that the patch replaces have gained error handling on the default branch in the mean time. The timings remain the same on my Windows machine: around 30ms for default branch, around 15ms with patch.
msg176376 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-25 17:18
> So this patch is a workaround for poor fseek performance in Microsoft's CRT, it doesn't cause performance issues on Linux but saves quite some milliseconds of startup time so I think it's worth the tradeoff.

I think some clarifying comments will be good in the code. In particular about the `dummy` variable.

> I'll also upload zipimport_speedup-v3.patch updated to apply to current default and with error handling for failing freads since the fseeks that the patch replaces have gained error handling on the default branch in the mean time.

Perhaps getc() requires error handling too.
msg178602 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-30 19:10
Catalin, are you going to continue?
msg178828 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2013-01-02 17:56
Yes Serhiy, I was planning to, lack of time followed by (just finished) vacation lead to a stall. Thanks for following up on this.

I should soon, probably this weekend, have an updated version addressing your comments.
msg179226 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2013-01-06 21:05
Attached v4 of patch with error checking for getc and some more comments.

A real world example of the speedup is Calibre (http://calibre-ebook.com/) which on Windows comes with its own code, its dependecies and Python's stdlib all bundled in a 40MB zip. With the patch the time to create a zipimporter instance from that zip drops from around 60ms to around 15ms.
msg179268 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-07 15:01
In general, the patch LGTM, however I can't try it on Windows, and on Linux it has no any performance effect. Can anyone try it on Windows?

I have re-uploaded the patch for review after converting it from UTF-16 and CRLF.
msg180312 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-20 20:40
Sorry, Brian. Raymond is an initiator of issue17004.
msg182230 - (view) Author: Roundup Robot (python-dev) Date: 2013-02-16 15:44
New changeset 088a14031998 by Serhiy Storchaka in branch 'default':
Issue #8745: Small speed up zipimport on Windows. Patch by Catalin Iacob.
http://hg.python.org/cpython/rev/088a14031998
msg182241 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-16 21:54
Thank you for contribution.
History
Date User Action Args
2013-02-16 21:54:44serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg182241

stage: patch review -> resolved
2013-02-16 15:44:34python-devsetnosy: + python-dev
messages: + msg182230
2013-01-20 20:40:58serhiy.storchakasetnosy: + rhettinger, - brian.curtin
messages: + msg180312
2013-01-20 19:23:48serhiy.storchakasetnosy: + brian.curtin
2013-01-07 15:01:49serhiy.storchakasetfiles: + zipimport-speedup-v4.patch

messages: + msg179268
2013-01-06 21:05:40catalin.iacobsetfiles: + zipimport-speedup-v4.patch

messages: + msg179226
2013-01-02 17:56:49catalin.iacobsetmessages: + msg178828
2012-12-30 19:10:09serhiy.storchakasetmessages: + msg178602
2012-12-29 22:10:45serhiy.storchakasetassignee: serhiy.storchaka
2012-11-25 17:18:08serhiy.storchakasetmessages: + msg176376
2012-11-25 15:41:13catalin.iacobsetfiles: + zipimport-speedup-v3.patch
2012-11-25 15:41:00catalin.iacobsetfiles: + attempt-fseek-seek_cur.patch

messages: + msg176367
2012-11-13 07:22:02eric.snowsetnosy: + eric.snow
2012-11-05 19:40:08serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg174934
2012-10-28 20:55:53catalin.iacobsetversions: + Python 3.4, - Python 3.3
2012-06-04 21:55:35catalin.iacobsetfiles: + zipimport_speedup-v2.patch
versions: + Python 3.3, - Python 3.2
nosy: + catalin.iacob

messages: + msg162300
2011-06-27 00:33:10brett.cannonsetassignee: brett.cannon -> (no value)
2010-05-20 21:09:04Goplatsetmessages: + msg106191
2010-05-18 11:35:34pitrousetassignee: brett.cannon
stage: patch review

nosy: + brett.cannon
versions: + Python 3.2, - Python 2.6
2010-05-18 10:09:12ysj.raysetnosy: + ysj.ray
messages: + msg105960
2010-05-18 06:08:09Goplatcreate