get rid of some main-thread IO (mobile)

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

(Blocks: 1 bug, {main-thread-io})

Trunk
Firefox 26
All
Android
main-thread-io
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

6 years ago
I'm also going to port something to OS.File.
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Whiteboard: [Snappy]
(Assignee)

Updated

6 years ago
Summary: don't use file.exists() when not necessary in mobile → don't use file.exists() when not necessary (mobile)
(Assignee)

Comment 1

6 years ago
Created attachment 710975 [details] [diff] [review]
Patch
Attachment #710975 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #710975 - Flags: review?(mark.finkle) → feedback?(mark.finkle)
(Assignee)

Comment 2

6 years ago
Created attachment 710990 [details] [diff] [review]
Patch v2

I've found other code that could be ported to use OS.File.
Should I use a lazy getter to import osfile.jsm and Task.jsm?
Attachment #710975 - Attachment is obsolete: true
Attachment #710975 - Flags: feedback?(mark.finkle)
Attachment #710990 - Flags: feedback?(mark.finkle)
Comment on attachment 710990 [details] [diff] [review]
Patch v2

In general this looks OK. I would like to see more use of lazy getters though.

I also don't like adding back the "self = this" pattern. We have been using function.bind(this) in new code and removing the old pattern. Can you use function/bind() as well?


Some specifics:

>diff --git a/mobile/android/chrome/content/aboutDownloads.js b/mobile/android/chrome/content/aboutDownloads.js

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+      let promise = OS.File.writeAtomic(aFile.path, array, {tmpPath: aFile.path + ".tmp"}).then(null, function onError(reason) {

nit: use a space inside the {}

{ tmpPath: aFile.path + ".tmp" }

>+        OS.File.writeAtomic(this._file.path, array, {tmpPath: this._file.path + ".tmp"});

Same

>+    Task.spawn(function() {
>+      let bytes = yield OS.File.read(aFile.path);
>+      let raw = new TextDecoder("UTF-8").decode(bytes) || "";
> 
>       try {
>         aCallback(JSON.parse(raw));
>       } catch (e) {
>         Cu.reportError("Distribution: Could not parse JSON: " + e);
>       }
>+    }).then(null, function onError(reason) {
>+      if (!(reason instanceof OS.File.Error && reason.becauseNoSuchFile)) {
>+        Cu.reportError("Distribution: Could not read from " + aFile.leafName + " file");
>+      }

Task is a new pattern for me. Why are we using it instead of the promise returned by OS.File.read ?
Are there docs on Task somewhere?
Attachment #710990 - Flags: feedback?(mark.finkle) → feedback+
(Assignee)

Comment 4

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Task is a new pattern for me. Why are we using it instead of the promise
> returned by OS.File.read ?
> Are there docs on Task somewhere?

It just improves readability, because it makes asynchronous operations look like synchronous ones.
There's some documentation here (http://dxr.mozilla.org/mozilla-central/toolkit/content/Task.jsm.html) and here (http://taskjs.org/).

I'll fix the other things, I've just a doubt about the lazy getters. I don't know when I should use them and when I shouldn't.
One rule of thumb for lazy getters is this: If the code is not used during startup, it's nice to use a lazy getter so we can defer the cost of loading and parsing the JS.
(Assignee)

Comment 6

6 years ago
Created attachment 712158 [details] [diff] [review]
Patch v3
Attachment #710990 - Attachment is obsolete: true
Attachment #712158 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 801603
Comment on attachment 712158 [details] [diff] [review]
Patch v3

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+XPCOMUtils.defineLazyGetter(this, "Task", function() {
>+  Cu.import("resource://gre/modules/Task.jsm");
>+  return DebuggerServer;

    return Task;

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

>diff --git a/mobile/android/chrome/content/downloads.js b/mobile/android/chrome/content/downloads.js

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

>diff --git a/mobile/android/components/AddonUpdateService.js b/mobile/android/components/AddonUpdateService.js

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>+XPCOMUtils.defineLazyGetter(this, "Task", function() {
>+  Cu.import("resource://gre/modules/Task.jsm");
>+  return DebuggerServer;

    return Task;

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

Let's get a Try Server run for the next patch. Also, request review from :bnicholson as well. He has worked in many of these files.
Attachment #712158 - Flags: review?(mark.finkle) → review-

Updated

6 years ago
Duplicate of this bug: 845601
Summary: don't use file.exists() when not necessary (mobile) → get rid of some main-thread IO (mobile)
Duplicate of this bug: 721786
Keywords: main-thread-io
(Assignee)

Comment 11

5 years ago
Created attachment 784476 [details] [diff] [review]
Patch v4

Try run: https://tbpl.mozilla.org/?tree=Try&rev=5558c370aa6b

There are contacts api tests failing, but I guess they're unrelated.
Attachment #712158 - Attachment is obsolete: true
Attachment #784476 - Flags: review?(mark.finkle)
Attachment #784476 - Flags: review?(bnicholson)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 801611
Comment on attachment 784476 [details] [diff] [review]
Patch v4

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

::: mobile/android/components/HelperAppDialog.js
@@ +132,5 @@
>        try {
>          // Remove the file so that it's not there when we ensure non-existence later;
>          // this is safe because for the file to exist, the user would have had to
>          // confirm that he wanted the file overwritten.
> +        file.remove(false);

I assume you're not changing this to OS.File.remove() because of a potential race with the non-existence check described in the comment -- is that correct?

::: mobile/android/components/SessionStore.js
@@ +1002,1 @@
>              notifyObservers("fail");

Move this line outside of the if/else block; notifyObservers() must be called unconditionally after attempting to restore.
Attachment #784476 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 785448 [details] [diff] [review]
Patch v5

(In reply to Brian Nicholson (:bnicholson) (gone through August 11) from comment #13)
> I assume you're not changing this to OS.File.remove() because of a potential
> race with the non-existence check described in the comment -- is that
> correct?

Exactly.
Attachment #784476 - Attachment is obsolete: true
Attachment #784476 - Flags: review?(mark.finkle)
Attachment #785448 - Flags: review?(mark.finkle)
Comment on attachment 785448 [details] [diff] [review]
Patch v5

Sorry for the delay in the review. Looks good.
Attachment #785448 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/b742229dc66c
Keywords: checkin-needed
Whiteboard: [Snappy] → [Snappy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b742229dc66c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy][fixed-in-fx-team] → [Snappy]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.