865387 - [OS.File] Read-ahead flag for Linux

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Comment 1

[Sankha Narayan Guria [:sankha]]

Yoric, assuming the way we do in bug 865389 for MacOS X, we will have to use the readahead function in fcntl.h for Linux, in way defined here?

http://linux.die.net/man/2/readahead

Comment 2

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

As far as I understand, we should rather use fadvise() with flag FADV_SEQUENTIAL.
Cc-ing Taras to correct me if I'm wrong.

Comment 3

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

However, sankha93, before proceeding, please synchronize with yzen, who has been working on the MacOS X version.

Comment 4

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> As far as I understand, we should rather use fadvise() with flag
> FADV_SEQUENTIAL.
> Cc-ing Taras to correct me if I'm wrong.

correct.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

FYI, I ran the benchmark on linux with fadvise and FADV_SEQUENTIAL (equivalent to the one for OSX) and here are the results:

*** Reading took: 117.56 cpu sec ***
*** Reading ahead took: 88.88 cpu sec ***

It works quite well on linux :)

Comment 6

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

Ahah :)

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Benchmark file

Attaching the benchmark file to make sure I'm doing the right thing there.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi, sankha93. Let me know if you are still working on it, if not I will gladly pick it up from you :)

Comment 9

[Sankha Narayan Guria [:sankha]]

Attachment: WIP patch

This is WIP patch. Two tests are failing in toolkit/components/osfile/tests/mochi/test_osfile_async.xul in the read_write_all() and duration() functions with a WorkerErrorEvent.

I am not sure why it is failing in read_write_all(), because just in the previous test read_write(), the OS.File.readTo() is being tested and it passes - and readahead is used by any read operation.

Comment 10

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

Comment on attachment 768141 [details] [diff] [review]
WIP patch

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

I probably won't have time to look at it this week.
You should try setting |OS.Shared.DEBUG = true| to get more logging information.

Comment 11

[Sankha Narayan Guria [:sankha]]

I tested with the same patch this time setting |OS.Shared.DEBUG = true| and I got this error message from the "posix_fadvise" FFI call.

> OS Agent Error while calling agent method TypeError: expected type int32_t, got (void 0) ffi@resource://gre/modules/osfile/osfile_shared_allthreads.jsm:940

But in the docs as given in http://linux.die.net/man/2/posix_fadvise it should return int. What is wrong?

Comment 12

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

"got (void 0)" means that you passed as argument |undefined|

Comment 13

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

This patch passes all tests.

Comment 14

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

Comment on attachment 772830 [details] [diff] [review]
patch v1

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

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +73,2 @@
>   */
> +let clone = OS.Shared.clone;

Could you split this in two patches?
- one patch that handles |clone| and the changes to |options|;
- another patch that handles the actual read-ahead flag.

@@ +546,5 @@
>   * to read.
> + * @param {JSON} options Optionally, an object that may contain the
> + * following fields:
> + * - {number|null} outExecutionDuration A container for the duration of the
> + * read operation.

Let's not document this here.

@@ +549,5 @@
> + * - {number|null} outExecutionDuration A container for the duration of the
> + * read operation.
> + * - {boolean} sequential - a flag that triggers a population of the page cache
> + * with data from a file so that subsequent reads from that file would not
> + * block on disk I/O. Note: |sequential| set to true by default.

"If |true| or unspecified, inform the system that the contents of the file will be read in order. Otherwise, make no such assumption."

@@ +561,5 @@
> +    // reference to |outExecutionDuration| option if it is passed.
> +    options = clone(options, ["outExecutionDuration"]);
> +    options.sequential = true;
> +  }
> +  //throw new Error(path);

I suspect you forgot to remove this.

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +193,5 @@
> +           refer(result, k, object);
> +         }
> +       }
> +       return result;
> +     };

File osfile_shared_allthreads.jsm has changed a bit. You'll probably need to pull and merge your changes.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +86,5 @@
> +       throw_on_negative("posix_fadvise",
> +         UnixFile.posix_fadvise(this.fd, offset, count,
> +          OS.Constants.libc.POSIX_FADV_SEQUENTIAL)
> +       );
> +     };

That code will work on Linux but not on platforms that do not define posix_fadvise. You should define File.prototype._unixSequential only on platforms for which posix_fadvise is defined.

@@ +106,5 @@
>        */
>       File.prototype._read = function _read(buffer, nbytes, options) {
> +       if (options.sequential && this._unixSequential) {
> +         this._unixSequential(0, nbytes);
> +       }

Let's change the logics a bit, now that we have removed options.sequential.

Comment 15

[Sankha Narayan Guria [:sankha]]

Attachment: handles options and clone (part 1)

Comment 16

[Sankha Narayan Guria [:sankha]]

Attachment: handles actual read-ahead (part 2)

Comment 17

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

Comment on attachment 794617 [details] [diff] [review]
handles actual read-ahead (part 2)

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

::: dom/system/OSFileConstants.cpp
@@ +12,5 @@
>  #if defined(XP_UNIX)
>  #include "unistd.h"
>  #include "dirent.h"
>  #include "sys/stat.h"
> +//#include <linux/fadvise.h>

Why is this commented out?

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +110,5 @@
>        */
>       File.prototype._read = function _read(buffer, nbytes, options) {
> +       if (options.sequential && this._unixSequential) {
> +         this._unixSequential(0, nbytes);
> +       }

I realize that I asked yzen to do the contrary, but now that it looks like only Linux will have sequential, let's simplify a little the code:
 please inline the contents of _unixSequential here.

Comment 18

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

Comment on attachment 794616 [details] [diff] [review]
handles options and clone (part 1)

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

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +98,2 @@
>   */
> +let clone = OS.Shared.clone;

Please use SharedAll.clone.

@@ +324,5 @@
>      // If |buffer| is a typed array and there is no |bytes| options, we
>      // need to extract the |byteLength| now, as it will be lost by
>      // communication
> +if (isTypedArray(buffer) && !("bytes" in options)) {
> +      // Preserve reference to option |outExecutionDuration|, if it is passed.

Nit: indentation.

@@ +360,5 @@
>    write: function write(buffer, options = {}) {
>      // If |buffer| is a typed array and there is no |bytes| options,
>      // we need to extract the |byteLength| now, as it will be lost
>      // by communication
>      if (isTypedArray(buffer) && (!options || !("bytes" in options))) {

You might was well remove the !options check.

@@ +597,5 @@
> + * - {boolean} sequential - a flag that triggers a population of the page cache
> + * with data from a file so that subsequent reads from that file would not
> + * block on disk I/O. If |true| or unspecified, inform the system that the
> + * contents of the file will be read in order. Otherwise, make no such
> + * assumption. Note: |sequential| set to true by default.

Could you re-add the @param line you have removed?
Also, please replace the final sentence with just "|true| by default."

@@ +608,5 @@
> +    // Copy |options| to avoid modifying the original object but preserve the
> +    // reference to |outExecutionDuration| option if it is passed.
> +    options = clone(options, ["outExecutionDuration"]);
> +    options.sequential = true;
> +  }

I believe that we can get rid of this change.

Comment 19

[Sankha Narayan Guria [:sankha]]

Attachment: handles options and clone (part 1)

Comment 20

[Sankha Narayan Guria [:sankha]]

Attachment: handles actual read-ahead (part 2)

Comment 21

[Sankha Narayan Guria [:sankha]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7937760b0fe1

Comment 22

[Sankha Narayan Guria [:sankha]]

Attachment: handles actual read-ahead (part 2)

Updated the #includes to prevent Mac OSX build failures.

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

Comment 23

[Sankha Narayan Guria [:sankha]]

Attachment: handles actual read-ahead (part 2)

Oops wrong file!

Comment 24

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

Comment on attachment 795423 [details] [diff] [review]
handles actual read-ahead (part 2)

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

Looks good, thanks.
Could you make the following change? No need to ask for another review just for that change.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +94,5 @@
>       File.prototype._read = function _read(buffer, nbytes, options) {
> +      // Populate the page cache with data from a file so the subsequent reads
> +      // from that file will not block on disk I/O.
> +       if ((options.sequential || !("sequential" in options)) &&
> +            UnixFile.posix_fadvise) {

Nit: I would put the condition |UnixFile.posix_fadvise| first.

Comment 25

[Sankha Narayan Guria [:sankha]]

Attachment: handles actual read-ahead (part 2)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d986a560fbdd
https://hg.mozilla.org/integration/fx-team/rev/19dd0130c6ae

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android test failures.
https://hg.mozilla.org/integration/fx-team/rev/1156c0bc98b1

https://tbpl.mozilla.org/php/getParsedLog.php?id=27014541&tree=Fx-Team

Comment 28

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

Sankha, are you planning to finish the work on this bug?

Comment 29

[Sankha Narayan Guria [:sankha]]

Attachment: handles actual read-ahead (part 2), v2

Updated the patch. Green on try this time.

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

Comment 30

[Sankha Narayan Guria [:sankha]]

Attachment: handles options and clone (part 1), rebased

rebased patch

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/328f4650ee57
https://hg.mozilla.org/integration/fx-team/rev/30d64411e9a3

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/328f4650ee57
https://hg.mozilla.org/mozilla-central/rev/30d64411e9a3

Comment 33

[Nils Maier [:nmaier]]

Comment on attachment 816205 [details] [diff] [review]
handles options and clone (part 1), rebased

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

This patch introduces a bug where you cannot |file.write(buffer, {bytes: bytes})| anymore.
Should I reopen or file a new bug.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +381,2 @@
>        options = clone(options, ["outExecutionDuration"]);
>        options.bytes = buffer.byteLength;

This isn't correct! This will always set |options.bytes = buffer.byteLength|, overriding whatever was in options.bytes before!

@@ -420,2 @@
>        options = clone(options, ["outExecutionDuration"]);
>        options.bytes = buffer.byteLength;

This was correct, to the extend that |options.bytes| doesn't get overridden!

Comment 34

[Nils Maier [:nmaier]]

(In reply to Nils Maier [:nmaier] from comment #33)
> Comment on attachment 816205 [details] [diff] [review]
> This patch introduces a bug where you cannot |file.write(buffer, {bytes:
> bytes})| anymore.
> Should I reopen or file a new bug.
> 

Filed as bug 926691.

Comment 35

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

Thanks.