784717 - [OS.File] Implement asynchronous directory walk

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Comment 1

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

Attachment: Companion testsuite

Comment 2

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

Attachment: Worker code

Comment 3

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

Attachment: Controller code

Comment 4

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

Note to Nathan: to understand these patches, you will need to get start with the dependencies.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 655292 [details] [diff] [review]
Controller code

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

Async programming makes my head hurt, but I think everything in here looks sound.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +600,5 @@
> +   * @resolves {OS.File.Entry}
> +   * @rejects {StopIteration} If all entries have already been visited.
> +   */
> +  next: function next() {
> +    let self = this;

I think it is a little more JavaScript-y to use .bind(this) on functions, but I will leave this up to you.  Certainly .bind can get tedious (e.g. your implementation of forEach)...

@@ +608,5 @@
> +    promise = promise.then(
> +      function withIterator(iterator) {
> +        return self._next(iterator);
> +      }
> +    );

Nit: I think the prevailing style is to });, rather than a more C-like brace style.  Please change this and other instances.  (And note that you were not completely consistent ;)

@@ +625,5 @@
> +  nextBatch: function nextBatch(size) {
> +    if (this._isClosed) {
> +      return Promise.resolve(null);
> +    }
> +    let self = this;

self is not needed here.

@@ +636,5 @@
> +    promise = promise.then(
> +      function withEntries(array) {
> +        if (array == null) {
> +          return null;
> +        }

I remember saying that nextBatch should return an array always, even if it was a zero-length array.  Please remove this null check.

I think that means the ._isClosed check above should return Promise.resolve([]), yes?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 655291 [details] [diff] [review]
Worker code

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

::: toolkit/components/osfile/osfile_async_worker.js
@@ +159,5 @@
> +       }
> +       if (!(file instanceof File.DirectoryIterator)) {
> +         throw new Error("file is not a directory iterator " + file.__proto__.toSource());
> +       }
> +       return f.call(file);

I am amused that none of the functions you pass to withDir actually name their parameter.  I think this is intentional, but is the |file| parameter going to be useful further down the road?

@@ +203,5 @@
>         open: function open(path, mode, options) {
>           let file = File.open(Type.path.fromMsg(path), mode, options);
>           return OpenedFiles.add(file);
>         },
> +       new_DirectoryIterator: function(path, options) {

Please give this function a name.

@@ +265,5 @@
> +               }
> +               throw x;
> +             }
> +           }
> +         );

Nit: });  And other places.

@@ +280,5 @@
> +             }
> +             if (result == null) {
> +               // We have reached the end
> +               OpenedDirectoryIterators.remove(dir);
> +             }

nextBatch shouldn't be returning null.  And this null check fails to stop result.map from throwing an error.  Please fix.

Comment 7

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

(In reply to Nathan Froyd (:froydnj) from comment #5)
> I think it is a little more JavaScript-y to use .bind(this) on functions,
> but I will leave this up to you.  Certainly .bind can get tedious (e.g. your
> implementation of forEach)...

Both tedious, slow and error-prone, so for the moment, I am avoiding it until someone r- my code because of this:)

> @@ +608,5 @@
> > +    promise = promise.then(
> > +      function withIterator(iterator) {
> > +        return self._next(iterator);
> > +      }
> > +    );
> 
> Nit: I think the prevailing style is to });, rather than a more C-like brace
> style.  Please change this and other instances.  (And note that you were not
> completely consistent ;)

Oops :)

> @@ +625,5 @@
> > +  nextBatch: function nextBatch(size) {
> > +    if (this._isClosed) {
> > +      return Promise.resolve(null);
> > +    }
> > +    let self = this;
> 
> self is not needed here.

Thanks.

> 
> @@ +636,5 @@
> > +    promise = promise.then(
> > +      function withEntries(array) {
> > +        if (array == null) {
> > +          return null;
> > +        }
> 
> I remember saying that nextBatch should return an array always, even if it
> was a zero-length array.  Please remove this null check.

Right, thanks.

> 
> I think that means the ._isClosed check above should return
> Promise.resolve([]), yes?

Indeed.

Comment 8

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

(In reply to Nathan Froyd (:froydnj) from comment #6)
> Comment on attachment 655291 [details] [diff] [review]
> Worker code
> 
> Review of attachment 655291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_async_worker.js
> @@ +159,5 @@
> > +       }
> > +       if (!(file instanceof File.DirectoryIterator)) {
> > +         throw new Error("file is not a directory iterator " + file.__proto__.toSource());
> > +       }
> > +       return f.call(file);
> 
> I am amused that none of the functions you pass to withDir actually name
> their parameter.  I think this is intentional, but is the |file| parameter
> going to be useful further down the road?

Well, there is no |file| parameter (|call|'s first argument sets |this|), so probably not :)


> @@ +203,5 @@
> >         open: function open(path, mode, options) {
> >           let file = File.open(Type.path.fromMsg(path), mode, options);
> >           return OpenedFiles.add(file);
> >         },
> > +       new_DirectoryIterator: function(path, options) {
> 
> Please give this function a name.
> 
> @@ +265,5 @@
> > +               }
> > +               throw x;
> > +             }
> > +           }
> > +         );
> 
> Nit: });  And other places.
> 
> @@ +280,5 @@
> > +             }
> > +             if (result == null) {
> > +               // We have reached the end
> > +               OpenedDirectoryIterators.remove(dir);
> > +             }
> 
> nextBatch shouldn't be returning null.  And this null check fails to stop
> result.map from throwing an error.  Please fix.

Ok.

Comment 9

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

Attachment: Controller code, v2

Comment 10

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

Attachment: Worker code, v2

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 656045 [details] [diff] [review]
Controller code, v2

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

This is a bit of a rubberstamp.  I assume that all the read/write stuff will need updating for the options.bytes changes made elsewhere, correct?

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +50,5 @@
> +
> +/**
> + * An implementation of queues (FIFO).
> + *
> + * The current implementation uses two arrays and runs in O(n * log(n)).

n log n?  Really?  No copies, no recursive calls, what is driving the log n factor here?  I'm not going to nitpick about this, but I find this very surprising.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 656050 [details] [diff] [review]
Worker code, v2

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

::: toolkit/components/osfile/osfile_async_worker.js
@@ +286,2 @@
>             }
>           );

Feel free to }); this. :)

Comment 13

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

(In reply to Nathan Froyd (:froydnj) from comment #11)
> Comment on attachment 656045 [details] [diff] [review]
> Controller code, v2
> 
> Review of attachment 656045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a bit of a rubberstamp.  I assume that all the read/write stuff will
> need updating for the options.bytes changes made elsewhere, correct?

Indeed. All part of bug 769191 and bug 777711.

> 
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +50,5 @@
> > +
> > +/**
> > + * An implementation of queues (FIFO).
> > + *
> > + * The current implementation uses two arrays and runs in O(n * log(n)).
> 
> n log n?  Really?  No copies, no recursive calls, what is driving the log n
> factor here?  I'm not going to nitpick about this, but I find this very
> surprising.

Array growth. I have not actually checked the implementation of array growth in SpiderMonkey, but the usual implementation is O(n * log(n)).

Comment 14

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

Attachment: Companion testsuite

Comment 15

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

Attachment: Worker code, v2

Comment 16

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

Attachment: Controller code, v2

Comment 17

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

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=cd81c2337d5b

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/44c79a5f7131
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b62202f9e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8332dd98c1cb

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/44c79a5f7131
https://hg.mozilla.org/mozilla-central/rev/90b62202f9e3
https://hg.mozilla.org/mozilla-central/rev/8332dd98c1cb