If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[OS.File] Read-ahead flag for Linux

RESOLVED FIXED in mozilla27

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Yoric, Assigned: sankha)

Tracking

(Blocks: 1 bug)

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async][Snappy])

Attachments

(3 attachments, 9 obsolete attachments)

4.29 KB, patch
Details | Diff | Splinter Review
3.74 KB, patch
Details | Diff | Splinter Review
12.71 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
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
Flags: needinfo?(dteller)
As far as I understand, we should rather use fadvise() with flag FADV_SEQUENTIAL.
Cc-ing Taras to correct me if I'm wrong.
Flags: needinfo?(dteller) → needinfo?(taras.mozilla)
However, sankha93, before proceeding, please synchronize with yzen, who has been working on the MacOS X version.

Comment 4

4 years ago
(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.
Flags: needinfo?(taras.mozilla)

Updated

4 years ago
Depends on: 865389
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 :)
Flags: needinfo?(dteller)
Ahah :)
Flags: needinfo?(dteller)
Created attachment 767436 [details] [diff] [review]
Benchmark file

Attaching the benchmark file to make sure I'm doing the right thing there.
Hi, sankha93. Let me know if you are still working on it, if not I will gladly pick it up from you :)
(Assignee)

Comment 9

4 years ago
Created attachment 768141 [details] [diff] [review]
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.
Assignee: nobody → sankha93
Status: NEW → ASSIGNED
Attachment #768141 - Flags: feedback?(dteller)
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.
Attachment #768141 - Flags: feedback?(dteller)
(Assignee)

Comment 11

4 years ago
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?
"got (void 0)" means that you passed as argument |undefined|
(Assignee)

Comment 13

4 years ago
Created attachment 772830 [details] [diff] [review]
patch v1

This patch passes all tests.
Attachment #768141 - Attachment is obsolete: true
Attachment #772830 - Flags: review?(dteller)
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.
Attachment #772830 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 15

4 years ago
Created attachment 794616 [details] [diff] [review]
handles options and clone (part 1)
Attachment #772830 - Attachment is obsolete: true
Attachment #794616 - Flags: review?(dteller)
(Assignee)

Comment 16

4 years ago
Created attachment 794617 [details] [diff] [review]
handles actual read-ahead (part 2)
Attachment #794617 - Flags: review?(dteller)
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.
Attachment #794617 - Flags: review?(dteller) → feedback+
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.
Attachment #794616 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 19

4 years ago
Created attachment 794658 [details] [diff] [review]
handles options and clone (part 1)
Attachment #794616 - Attachment is obsolete: true
Attachment #794658 - Flags: review?(dteller)
(Assignee)

Comment 20

4 years ago
Created attachment 794660 [details] [diff] [review]
handles actual read-ahead (part 2)
Attachment #794617 - Attachment is obsolete: true
Attachment #794660 - Flags: review?(dteller)
(Assignee)

Comment 21

4 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7937760b0fe1
(Assignee)

Comment 22

4 years ago
Created attachment 795421 [details] [diff] [review]
handles actual read-ahead (part 2)

Updated the #includes to prevent Mac OSX build failures.

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=077635e4a3ab
Attachment #794660 - Attachment is obsolete: true
Attachment #794660 - Flags: review?(dteller)
Attachment #795421 - Flags: review?(dteller)
(Assignee)

Comment 23

4 years ago
Created attachment 795423 [details] [diff] [review]
handles actual read-ahead (part 2)

Oops wrong file!
Attachment #795421 - Attachment is obsolete: true
Attachment #795421 - Flags: review?(dteller)
Attachment #795423 - Flags: review?(dteller)
Attachment #794658 - Flags: review?(dteller) → review+
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.
Attachment #795423 - Flags: review?(dteller) → review+
(Assignee)

Comment 25

4 years ago
Created attachment 795475 [details] [diff] [review]
handles actual read-ahead (part 2)
Attachment #795423 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d986a560fbdd
https://hg.mozilla.org/integration/fx-team/rev/19dd0130c6ae
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
Whiteboard: [fixed-in-fx-team]
Sankha, are you planning to finish the work on this bug?
Flags: needinfo?(sankha93)
Whiteboard: [Async][Snappy]
(Assignee)

Comment 29

4 years ago
Created attachment 816043 [details] [diff] [review]
handles actual read-ahead (part 2), v2

Updated the patch. Green on try this time.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8545d8acd913
Attachment #795475 - Attachment is obsolete: true
Flags: needinfo?(sankha93)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 30

4 years ago
Created attachment 816205 [details] [diff] [review]
handles options and clone (part 1), rebased

rebased patch
Attachment #794658 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/328f4650ee57
https://hg.mozilla.org/integration/fx-team/rev/30d64411e9a3
Keywords: checkin-needed
Whiteboard: [Async][Snappy] → [Async][Snappy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/328f4650ee57
https://hg.mozilla.org/mozilla-central/rev/30d64411e9a3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Async][Snappy][fixed-in-fx-team] → [Async][Snappy]
Target Milestone: --- → mozilla27
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!

Updated

4 years ago
Blocks: 926691
(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.
Thanks.

Updated

4 years ago
Blocks: 928239
You need to log in before you can comment on or make changes to this bug.