989326 - Loader.jsm should neither flush nor perform main thread I/O

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

In Loader.jsm, method _writeFile flushes after every write and performs accidental main thread I/O. It would be more efficient to use OS.File.writeAtomic instead.

Comment 1

[Girish Sharma [:Optimizer]]

Just a note that _writeFile method is part of SRCDIR loader, which is only used by devtools developer who are hacking on devtools code and want auto reload of code.

Comment 2

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

It's still a blight on the code :)

More seriously, this appeared when I was searching for sources of accidental main thread I/O, so I filed the bug. The fix would be quite trivial.

If someone is interested in this bug, the code to fix is here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/Loader.jsm?from=Loader.jsm#177

Comment 3

[Manish Goregaokar [:manishearth]]

Assigning to vedavidh as per in-person request at IITKGP MozSetup.

Comment 4

[vedavidh]

Attachment: async.patch

I think I fixed this, can you have a look?

Comment 5

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

Comment on attachment 8399066 [details] [diff] [review]
async.patch

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

Good start, thanks for the patch.
Could you make the changes below?

::: toolkit/devtools/Loader.jsm
@@ +175,5 @@
>  
> + _writeFile: function(filename, data) {
> +    let encoder = new TextEncoder();
> +    let array = encoder.encoder(data);
> +    let promise = OS.File.writeAtomic(filename,array);

Just |OS.File.writeAtomic(filename, data, { encoding: "utf-8"})| will do the trick.

@@ +176,5 @@
> + _writeFile: function(filename, data) {
> +    let encoder = new TextEncoder();
> +    let array = encoder.encoder(data);
> +    let promise = OS.File.writeAtomic(filename,array);
> +    promise.then(,function() {

That's a syntax error. You should pass |null| as first argument of |then|.

@@ +177,5 @@
> +    let encoder = new TextEncoder();
> +    let array = encoder.encoder(data);
> +    let promise = OS.File.writeAtomic(filename,array);
> +    promise.then(,function() {
> +      new Error("Couldn't write manifest: " + filename + "\n");

Don't just create the error, you should throw it.
Also, let's take the opportunity to make the message more precise:

promise.then(null, function(ex) {
 new Error("Couldn't write manifest: " + ex + "\n");
});

@@ +182,2 @@
>      });
> +    return promise;

You should return the result of the call to |then|, instead of just |promise|.

Comment 6

[vedavidh]

Attachment: patch v2

i have made the changes that you have asked for. could you please  go through this new version and check it?

Comment 7

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

Comment on attachment 8399078 [details] [diff] [review]
patch v2

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

Looks good to me, thanks for the patch.
Don't forget to fold both patches in one.
Try Server results: https://tbpl.mozilla.org/?tree=Try&rev=4bf1a5dae897

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8399078 [details] [diff] [review]
patch v2

Looks like this is a patch on top of patch v1, so calling it "patch v2" is a bit misleading :) You should make sure to attach a patch that combines both for someone to check it in.

Comment 9

[Manish Goregaokar [:manishearth]]

@Vedavidh, to fold the patches, do `hg qpop -a` (to pop all patches from the queue), then `hg qpush` (to put the first patch back on), and then `hg qfold nameofsecondpatch.patch`.

Edit the patch later ad make sure the commit details are consistent.

Comment 10

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

The tests look good. Vedavidh, once you have uploaded a folded patch, we'll be able to land this.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8399066 [details] [diff] [review]
async.patch

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

::: toolkit/devtools/Loader.jsm
@@ +172,5 @@
>      });
>      return deferred.promise;
>    },
>  
> + _writeFile: function(filename, data) {

The indentation here is now off, please fix it.

Comment 12

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

Vedavidh? Could you finish your patch?

Comment 13

[vedavidh]

Sorryy I couldn't make the change

Comment 14

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

Do you need assistance, vedavidh?

Comment 15

[Manish Goregaokar [:manishearth]]

Hm, you can ask for help here, you know :)

What's the issue? Do you have the patches in your local repo?

Comment 16

[vedavidh]

actually i have formatted my system due to some problem

Comment 17

[Manish Goregaokar [:manishearth]]

Hm. You can install Mercurial (sudo apt-get install mercurial on Ubuntu), and re-make the patch:

Download http://ftp.mozilla.org/pub/mozilla.org/firefox/bundles/mozilla-central.hg
mkdir mozilla-central
hg init mozilla-central
cd mozilla-central
hg unbundle /path/to/your/bundle.hg

Now download the two patches, and run hg qimport /path/to/patch

Now hg qpop -a, hg qpush nameoffirstpatch.patch, then hg qfold nameofsecondpatch.patch

Now make the edit to the file in comment 11, and `hg qref` to refresh the patch. Upload the new patch from mozilla-central/.hg/patches/nameofpatch.patch

Comment 18

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

vedavidh, the work is almost complete, so it would be great if you could finish it.

Comment 19

[Mike Hoye [:mhoye]]

Hi, Vedavidh - you're really close here, is there anything we can do to help you carry this over the line?

Comment 20

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

Looks like Vedavidh is not working on this bug anymore. Uniassigning.

Comment 21

[Akshendra Pratap Singh]

Attachment: 989326_loader_jsmNoFlush.patch

Comment 22

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

Comment on attachment 8515478 [details] [diff] [review]
989326_loader_jsmNoFlush.patch

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

Looks almost good.

::: toolkit/devtools/Loader.jsm
@@ +217,5 @@
>      });
>      return deferred.promise;
>    },
>  
> +  _writeFile: function(filename, data) {    

Nit: Could you remove the whitespace at the end of this line?

@@ +220,5 @@
>  
> +  _writeFile: function(filename, data) {    
> +    let promise = OS.File.writeAtomic(filename, data, {encoding: "utf-8"});
> +    return promise.then(null, (ex) => {
> +      new Error("Couldn't write manifest: " + ex + "\n");

Remove the curly braces, otherwise, the error handler will just produce `undefined`.

Comment 23

[Akshendra Pratap Singh]

Attachment: 989326_loader_jsmNoFlush.patch

Comment 24

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

Comment on attachment 8515933 [details] [diff] [review]
989326_loader_jsmNoFlush.patch

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

Looks good to me, thanks.

Comment 25

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

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c4afb1bd8719

Comment 26

[Akshendra Pratap Singh]

Please assign that to me.

Comment 27

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

That looks good. Are you ready to land it?

Comment 28

[Akshendra Pratap Singh]

How can I do that?

Comment 29

[Akshendra Pratap Singh]

How to do that?

Comment 30

[Akshendra Pratap Singh]

How to do that?

Comment 31

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

Just ask me :)

Comment 32

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/3363feb38c14

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3363feb38c14