Closed Bug 1404887 Opened 2 years ago Closed 2 years ago

mozfile.extract_zip reads entire zip entries into memory

Categories

(Testing :: Mozbase, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ted, Assigned: froydnj)

References

Details

Attachments

(1 file, 1 obsolete file)

bkelly linked a WPT test run that failed trying to symbolicate an assertion stack:
https://treeherder.mozilla.org/logviewer.html#?job_id=133188470&repo=try&lineNumber=36760

It looks like Python OOMed trying to extract the symbols zip in mozfile.extract_zip:
https://dxr.mozilla.org/mozilla-central/rev/41286177c59c74ec37961b1edea34cc90d6f6dc5/testing/mozbase/mozfile/mozfile/mozfile.py#73

That function reads each zip entry into memory to extract it to disk, which is unnecessary. This function was probably written prior to us requiring Python 2.6, but now that we require 2.7 we can either use ZipFile.extract:
https://docs.python.org/2/library/zipfile.html#zipfile.ZipFile.extract

or rewrite this whole function to use ZipFile.extractall:
https://docs.python.org/2/library/zipfile.html#zipfile.ZipFile.extractall
Attached patch use ZipFile.extract in mozfile (obsolete) — Splinter Review
Reading the whole zip entry into memory is inefficient and can cause
OOMs if the entry is large enough.  Let the ZipFile object choose the
most efficient extraction strategy instead.

We'll wait until Ted unsets the not-accepting-reviews flag on his profile. :)
Attachment #8914286 - Flags: review?(ted)
Assignee: nobody → nfroyd
Comment on attachment 8914286 [details] [diff] [review]
use ZipFile.extract in mozfile

Review of attachment 8914286 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8914286 - Flags: review?(ted) → review+
Turns out ZipFile.extract does all the logic we were doing to makedirs, etc.,
and we needed to pass in the directory to extract to, not the filename to write
into.  This is a nicer patch in some ways!
Attachment #8921202 - Flags: review?(ted)
Attachment #8914286 - Attachment is obsolete: true
Comment on attachment 8921202 [details] [diff] [review]
use ZipFile.extract in mozfile

Review of attachment 8921202 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8921202 - Flags: review?(ted) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a828043c22d
use ZipFile.extract in mozfile; r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/6a828043c22d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.