Closed Bug 927560 Opened 6 years ago Closed 6 years ago

[OS.File] Make it possible to shutdown osfile_async_worker.js when it's not needed

Categories

(Toolkit :: OS.File, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: fabrice, Assigned: Yoric)

References

Details

(Keywords: perf, Whiteboard: [c=memory p= s= u=1.2] [MemShrink:P1])

Attachments

(4 files, 3 obsolete files)

Workers are quite memory hungry and that's a problem on b2g. As much as we'd like to use OS.File goodness, wasting permanently 1MB to read a file is not something we can live with.
Whiteboard: [MemShrink]
Three possible strategies:
1. Safe and transparent – the worker is shutdown n milliseconds after its last use (which means that any File/DirectoryIterator needs to be closed);
2. Safe and manual – publish an method for closing the worker, the method throws if there is any instance of File/DirectoryIterator around;
3. Unsafe – publish a method for closing the worker, it auto-closes an instance of File/DirectoryIterator.

What's your preference? I believe that I would go for 2.
Flags: needinfo?(fabrice)
Summary: Shutdown osfile_async_worker.js when it's not needed → [OS.File] Shutdown osfile_async_worker.js when it's not needed
2. sounds good to me also!
Flags: needinfo?(fabrice)
Keywords: perf
Whiteboard: [MemShrink] → [c=memory p= s= u=] [MemShrink]
Assignee: nobody → dteller
Attached patch OS.File.resetWorker (obsolete) — Splinter Review
Adding a new API |OS.File.resetWorker|.
Attachment #818454 - Flags: review?(nfroyd)
Attached patch TestsSplinter Review
Companion tests.
I took the opportunity to remove test_system_shutdown and move it to a xpcshell-test, as I needed to run it a number of times to ensure I hadn't broken it.
Attachment #818455 - Flags: review?(nfroyd)
Blocks a blocker, so copying flags/priority.
blocking-b2g: --- → koi+
Whiteboard: [c=memory p= s= u=] [MemShrink] → [c=memory p= s= u=] [MemShrink:P1]
Severity: normal → blocker
Attachment #818454 - Flags: review?(nfroyd) → review+
Attachment #818455 - Flags: review?(nfroyd) → review+
[/c/src-gecko/inbound]$ qimp 927560
Fetching... done
Parsing... done
1: OS.File.resetWorker
  nfroyd: review+
2: Tests
  nfroyd: review+
adding 927560 to series file
renamed 927560 -> osfile.shutdown.test
adding osfile.shutdown to series file
applying osfile.shutdown
patching file toolkit/components/osfile/modules/osfile_async_worker.js
Hunk #3 FAILED at 299
1 out of 4 hunks FAILED -- saving rejects to file toolkit/components/osfile/modules/osfile_async_worker.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh osfile.shutdown

Could you rebase and re-mark for checkin please? :-)
Keywords: checkin-needed
Rebased.
Attachment #818454 - Attachment is obsolete: true
Attachment #821641 - Flags: review+
Had to rebase osfile.shutdown.test too, but looked trivial, so just landed it to save another back and forth.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c275ba558e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3efb38280835
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5c275ba558e
https://hg.mozilla.org/mozilla-central/rev/3efb38280835
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
This will need an updated patch for aurora/koi uplift:

[/c/src-gecko/aurora]$ transplant e5c275ba558e
searching for changes
applying e5c275ba558e
patching file toolkit/components/osfile/modules/osfile_async_front.jsm
Hunk #1 FAILED at 92
Hunk #3 FAILED at 988
2 out of 3 hunks FAILED -- saving rejects to file toolkit/components/osfile/modules/osfile_async_front.jsm.rej
patching file toolkit/components/osfile/modules/osfile_async_worker.js
Hunk #1 FAILED at 61
Hunk #2 FAILED at 186
Hunk #3 FAILED at 281
Hunk #4 FAILED at 328
4 out of 4 hunks FAILED -- saving rejects to file toolkit/components/osfile/modules/osfile_async_worker.js.rej
patch failed to apply
Whiteboard: [c=memory p= s= u=] [MemShrink:P1] → [needs-aurora-patch] [c=memory p= s= u=] [MemShrink:P1]
Flags: needinfo?(dteller)
Whiteboard: [needs-aurora-patch] [c=memory p= s= u=] [MemShrink:P1] → [c=memory p= s= u=] [MemShrink:P1]
Attachment #823311 - Flags: review+
Flags: needinfo?(dteller)
Attached patch Tests - rebased for 26 (obsolete) — Splinter Review
Attachment #823313 - Flags: review+
So, I have attached patches rebased for 26.
However, until we know how we are going to use these patches, I'm not 100% sure we should land them.
(No uplifting yet due to comment 15)
s/No/Not/
Summary: [OS.File] Shutdown osfile_async_worker.js when it's not needed → [OS.File] Make it possible to shutdown osfile_async_worker.js when it's not needed
Setting status-firefox26 to wontfix since this isn't going to be landing on mozilla-beta at this point.
(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> So, I have attached patches rebased for 26.
> However, until we know how we are going to use these patches, I'm not 100%
> sure we should land them.

I assume that the blocking status of this and bug 921659 will be revisited then?
This is in my queue for landing on b2g26, but I will wait until the status of this and bug 921659 are sorted out.
It is my understanding that we don't need to uplift this anymore, so removing block.
No longer blocks: 921659
blocking-b2g: koi+ → 1.4?
Yeah, I don't think we need this anymore.
blocking-b2g: 1.4? → ---
Blocks: 944859
blocking-b2g: --- → koi+
I'm back in front of bugzilla. Investigating.
Flags: needinfo?(dteller)
Whiteboard: [c=memory p= s= u=] [MemShrink:P1] → [c=memory p= s= u=1.2] [MemShrink:P1]
Attached patch Tests - rebased for 26, v2 (obsolete) — Splinter Review
Looks like the problem was just a timeout that was too short. By increasing the timeout, the test seems to pass.
Attachment #823313 - Attachment is obsolete: true
Attachment #8346759 - Flags: review+
Sorry, forgot a qref. That's the latest version.
Attachment #8346759 - Attachment is obsolete: true
Attachment #8347220 - Flags: review+
The test was still failing on b2g26. Maybe bug 944859 is why? Anyway, per IRC conversation with Yoric, the test is just going to be skipped.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b047544c652a

10:30:33 AM - Yoric: RyanVM: What seems to happen is that some code that prints to the nsIConsole doesn't print anything to that console in your tests.
10:32:21 AM - Yoric: Note that this test only tests whether we print a warning.
10:32:32 AM - Yoric: So I wouldn't be infuriated if we just skipped it.
You need to log in before you can comment on or make changes to this bug.