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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[[:fabrice] Fabrice Desré]

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.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 2

[[:fabrice] Fabrice Desré]

2. sounds good to me also!

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: OS.File.resetWorker

Adding a new API |OS.File.resetWorker|.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Tests

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Blocks a blocker, so copying flags/priority.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=ba32ddfe3058

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Adding other Try platforms.
Try: https://tbpl.mozilla.org/?tree=Try&rev=69b9b2163dc2

Comment 8

[Ed Morley [:emorley]]

[/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? :-)

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: OS.File.resetWorker, v2

Rebased.

Comment 10

[Ed Morley [:emorley]]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5c275ba558e
https://hg.mozilla.org/mozilla-central/rev/3efb38280835

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: OS.File.resetWorker - rebased for 26

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Tests - rebased for 26

Comment 15

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 16

[Ed Morley [:emorley]]

(No uplifting yet due to comment 15)

Comment 17

[Ed Morley [:emorley]]

s/No/Not/

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Setting status-firefox26 to wontfix since this isn't going to be landing on mozilla-beta at this point.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This is in my queue for landing on b2g26, but I will wait until the status of this and bug 921659 are sorted out.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

It is my understanding that we don't need to uplift this anymore, so removing block.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, I don't think we need this anymore.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7c90ac565923
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5c7962855ea7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ad7cf694eaac
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/aa3b9dc8c0e0

https://tbpl.mozilla.org/php/getParsedLog.php?id=31563127&tree=Mozilla-B2g26-v1.2

Comment 25

[David Teller [:Yoric] - still alive but not very active]

I'm back in front of bugzilla. Investigating.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: Tests - rebased for 26, v2

Looks like the problem was just a timeout that was too short. By increasing the timeout, the test seems to pass.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=25a038a98efc

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Attachment: Tests - rebased for 26, v3

Sorry, forgot a qref. That's the latest version.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4f8e819f8644
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/66fd5784afd9

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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.