Efficient JS File API - Unix back-end

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({perf})

unspecified
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 35 obsolete attachments)

1.47 KB, patch
Details | Diff | Splinter Review
11.81 KB, patch
Details | Diff | Splinter Review
14.03 KB, patch
Details | Diff | Splinter Review
14.75 KB, patch
Details | Diff | Splinter Review
Implement the JS File API backend.

A few considerations:
- Linux/Android and BSD/MacOS differ on a number of subtle-yet-interesting points (e.g. Linux/Android has a number of `openat`-style system calls optimized for opened directories, BSD/MacOS has optimized tree walking, additional `fstat` properties and `open` flags, etc.);
- this back-end needs to handle Unix-style permissions;
- Unix-style file properties is rather simple, as `fstat` extracts plenty of meaningful information in one operation;
- in some contexts, information may be obtained through less-expensive operations, e.g. determining whether a file is a directory while walking a directory.
Assignee: general → dteller
Blocks: 707676
Created attachment 580863 [details] [diff] [review]
Preview 2.

Still very much a work in progress.

Since previous preview:
- introduce a "native path" type, as suggested – as a typedef, though, I believe people would lynch me if I intend to add a new hierarchy of strings;
- removed a few functions/methods that had become useless by the way of this change;
- implemented a public |fstat| API;
- removed methods for reading a complete file, as expected;
- renamed |Rmdir| => |Remove|, |Rmcontents| => |Clear|;
- collapsed the variants of |Move|.

Mike, do you think that this |Resolve|/|Unresolve| API is meaningful? Or should we assume that a directory is always |Resolved| and should therefore be closed as early as possible?
Attachment #580863 - Flags: feedback?(mh+mozilla)

Comment 2

6 years ago
Lets get moving on landing this(ie switch to r?). Is it feasible to land something before we branch on Dec 20?
(In reply to Taras Glek (:taras) from comment #2)
> Lets get moving on landing this(ie switch to r?). Is it feasible to land
> something before we branch on Dec 20?

I can prioritize this, but the Windows version is lagging.

Comment 4

6 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> (In reply to Taras Glek (:taras) from comment #2)
> > Lets get moving on landing this(ie switch to r?). Is it feasible to land
> > something before we branch on Dec 20?
> 
> I can prioritize this, but the Windows version is lagging.

please do
Created attachment 581986 [details] [diff] [review]
Prev 3. Unix back-end.

I attach a version of the Unix back-end.

The architecture of RawFile should not change much. I still need to implement the following features:
- changing attributes (chown, chmod, chgrp);
- truncate;
- dup (should be useful for clean multi-threaded operations);
- in the future, play nicely with AIO.

By opposition, RawDirectory is still at the stage of untested, early work in progress. The architecture of RawDirectory remains a big question. I am very interested in feedback about |Resolve|/|Unresolve|. The main reason for including these functions are for users who could be tempted to cache many directories for later use – something that we do with |nsIFile| atm. However, at JS-level, we have a data structure to represent paths. I tend to believe that we should recommend that users should never hold onto a RawDirectory and simply |Resolve| all directories on platforms for which it matters. To discourage this, at JS-level, it would be quite easy to display warnings if a directory (or a file) remains open for more than, say, 30 minutes.

Finally, we are missing:
- creation of temporary files;
- |copy|, |touch| a file in a RawDirectory;
- |move|, |copy|, |rm|, |touch| an arbitrary file in the fs;
- getting the contents of a directory (with platform-specific info).
Attachment #580863 - Attachment is obsolete: true
Attachment #581986 - Flags: review?
Attachment #580863 - Flags: feedback?(mh+mozilla)
Attachment #581986 - Flags: review? → review?(taras.mozilla)
Created attachment 581988 [details] [diff] [review]
Prev 3. Back-end common files.
Attachment #581988 - Flags: review?(taras.mozilla)

Comment 7

6 years ago
Comment on attachment 581986 [details] [diff] [review]
Prev 3. Unix back-end.

Break this up atleast into 3 patches:
c/c++ bits
basic js bits
more js bitss
Attachment #581986 - Flags: review?(taras.mozilla) → review-
Created attachment 582523 [details] [diff] [review]
Prev 4. Unix back-end.

Here's the same patch, minus the first few lines, which had been included by error. Sorry about that.
Attachment #581986 - Attachment is obsolete: true
Attachment #582523 - Flags: review?(taras.mozilla)

Comment 9

6 years ago
Comment on attachment 582523 [details] [diff] [review]
Prev 4. Unix back-end.

This seems to be doing a lot for a thin layer. I think we should be passing all of the flags from JS. Forcing flags to be defined in C++ makes it impossible for addons to utilize a flag that we didn't provide.
Attachment #582523 - Flags: review?(taras.mozilla) → review-

Comment 10

6 years ago
Comment on attachment 581988 [details] [diff] [review]
Prev 3. Back-end common files.

-ing since i -sed part4. This is not minimal enough.
Attachment #581988 - Flags: review?(taras.mozilla) → review-
(In reply to Taras Glek (:taras) from comment #9)
> Comment on attachment 582523 [details] [diff] [review]
> Prev 4. Unix back-end.
> 
> This seems to be doing a lot for a thin layer.

I am not really sure how I could do less while still giving access to OS-accelerated features.

> I think we should be passing
> all of the flags from JS. Forcing flags to be defined in C++ makes it
> impossible for addons to utilize a flag that we didn't provide.

Ok, I will refactor |OpenFlags| to allow that.

(In reply to Taras Glek (:taras) from comment #10)
> Comment on attachment 581988 [details] [diff] [review]
> Prev 3. Back-end common files.
> 
> -ing since i -sed part4. This is not minimal enough.

Just to be sure I understand: you want me to split the patch, is that it?

Comment 12

6 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> (In reply to Taras Glek (:taras) from comment #9)
> > Comment on attachment 582523 [details] [diff] [review]
> > Prev 4. Unix back-end.
> > 
> > This seems to be doing a lot for a thin layer.
> 
> I am not really sure how I could do less while still giving access to
> OS-accelerated features.

I think the api shouldn't provide much more than a syscall wrapper. Then the js side should figure out which syscall to use and what parameters to pass.

> 
> > I think we should be passing
> > all of the flags from JS. Forcing flags to be defined in C++ makes it
> > impossible for addons to utilize a flag that we didn't provide.
> 
> Ok, I will refactor |OpenFlags| to allow that.
> 
> (In reply to Taras Glek (:taras) from comment #10)
> > Comment on attachment 581988 [details] [diff] [review]
> > Prev 3. Back-end common files.
> > 
> > -ing since i -sed part4. This is not minimal enough.
> 
> Just to be sure I understand: you want me to split the patch, is that it?

yes. I like patches split up to be <15KB each(when reasonable).
(In reply to Taras Glek (:taras) from comment #12)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> > (In reply to Taras Glek (:taras) from comment #9)
> > > Comment on attachment 582523 [details] [diff] [review]
> > > Prev 4. Unix back-end.
> > > 
> > > This seems to be doing a lot for a thin layer.
> > 
> > I am not really sure how I could do less while still giving access to
> > OS-accelerated features.
> 
> I think the api shouldn't provide much more than a syscall wrapper. Then the
> js side should figure out which syscall to use and what parameters to pass.

We could put all the logics into JS. I am not quite clear what we would gain, but I see a number of potential issues, which have made me choose this slightly thicker back-end:
- the resulting API is usable only by JS, not by C++;
- we lose performance, both in the implementation itself, and at boundaries (e.g. many Windows data structures are 64 bits or 128 bits, so we have to represent them as arrays in JS);
- we make it much easier to have resource leaks
- we lose some useful compile-time checking.

If you strongly believe that I should go for the syscall wrapper, I will go that way, but I believe that the current approach is better.

> yes. I like patches split up to be <15KB each(when reasonable).

Ok, will do.

Comment 14

6 years ago
Just to be clear: I strongly believe that we should go for the syscall wrapper approach.
Sure. Let me just finish cleaning up my patch queue and I'll start this new strategy.
Depends on: 739740
Created attachment 609838 [details] [diff] [review]
Exporting libc constants to JS

Attaching a first draft. This module exports a global object |OS.Constants.libc|, with many (all?) of the important libc constants. I envision that this module can be later extended with additional constants as needed.
Created attachment 609850 [details] [diff] [review]
Unix synchronous back-end

Attaching a fully revamped version of the Unix back-end. As discussed with Taras, this back-end is synchronous for the moment, and designed to be used only from a chrome worker thread. Once we have reached the stage at which this synchronous back-end works to satisfaction, we will build the asynchronous back-end on top of it. And only once the asynchronous back-end works will we start working first on the Windows back-end, then on the actual user-facing API.
Attachment #609850 - Flags: feedback?(mh+mozilla)
Created attachment 609852 [details] [diff] [review]
Companion testsuite
Attachment #581988 - Attachment is obsolete: true
Attachment #582523 - Attachment is obsolete: true
Attachment #609838 - Attachment is obsolete: true
Attachment #609850 - Flags: feedback?(poirot.alex)
Comment on attachment 609850 [details] [diff] [review]
Unix synchronous back-end

Nice. I just can't f- any js-ctypes patch!!

Wouldn't it be easier to use `new Type` instead of this 3th levels indirection?
  declareType > aDeclareType > gDeclareType > new Type()
Especially when, at the end, we do want a Type instance at the end.
(It would simplify code comprehension and allow removing init arguments that aren't used)
Similar comment apply to declareFFI, we might call gDeclareFFI directly.

Otherwise it looks really nice, js-ctypes burden doesn't look that bad in this library!
We are trying to provide some js-ctypes helpers in order to help addon developers start using it. And I think that your experience around js-ctype is really valuable. We may just take your helper methods and offer them in a ctype helper jetpack module!

What's the plan next? Would you build some cross platform library on top of that? I'm saying that because Jetpack FS API is cross platform, so that we would have to to this job, if you do not plan to do it.

Finally, what about CPU/memory performances? Especially compared to current xpcom components.

I can't wait to get other OSes and asynchronous support :)
Attachment #609850 - Flags: feedback?(poirot.alex) → feedback+
(In reply to Alexandre Poirot (:ochameau) from comment #19)
> Comment on attachment 609850 [details] [diff] [review]
> Unix synchronous back-end
> 
> Nice. I just can't f- any js-ctypes patch!!
> 
> Wouldn't it be easier to use `new Type` instead of this 3th levels
> indirection?
>   declareType > aDeclareType > gDeclareType > new Type()
> Especially when, at the end, we do want a Type instance at the end.
> (It would simplify code comprehension and allow removing init arguments that
> aren't used)

For Type, you are most likely right.

> Similar comment apply to declareFFI, we might call gDeclareFFI directly.

For declareFFI, it is a little more complicated. But I'm cheating, because I already have the implementation of the next bug at hand, and this argument serves for the threaded back-end. The main thread uses its own |declareFFI| to declare a function that checks and serializes arguments, deserializes the result and uses a Promise to synchronize, while the worker thread uses its own |declareFFI| to declare a function that deserializes arguments, performs the ffi call, serializes the result and posts it back to the main thread.

> Otherwise it looks really nice, js-ctypes burden doesn't look that bad in
> this library!
> We are trying to provide some js-ctypes helpers in order to help addon
> developers start using it. And I think that your experience around js-ctype
> is really valuable. We may just take your helper methods and offer them in a
> ctype helper jetpack module!

This would be with pleasure. Do you want to open a bug on extracting the relevant part and making them usable by Jetpack?

> What's the plan next? Would you build some cross platform library on top of
> that? I'm saying that because Jetpack FS API is cross platform, so that we
> would have to to this job, if you do not plan to do it.

Definitely planned. I used to have one, but it was scrapped in favor of this design, which is expected to be more hackable.

> Finally, what about CPU/memory performances? Especially compared to current
> xpcom components.

Untested yet. I suspect CPU/memory will be somewhat worse due to all conversions, but there should be less system calls, which we consider more important atm.
 
> I can't wait to get other OSes and asynchronous support :)

Thanks :)
Comment on attachment 609850 [details] [diff] [review]
Unix synchronous back-end

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

::: toolkit/components/osfile/osfile_unix.jsm
@@ +536,5 @@
> +                    /*dest*/   Types.string,
> +                    /*flag*/   Types.int);
> +
> +       UnixFile.lockf =
> +         declareFFI("lockf", ctypes.default_abi,

I'm not convinced it's a good idea to expose file locking functions.

@@ +555,5 @@
> +                    /*return*/ Types.string,
> +                    /*template*/Types.string);
> +
> +       UnixFile.mkstmp =
> +         declareFFI("mkstmp", ctypes.default_abi,

didn't you mean mkstemp ?
Attachment #609850 - Flags: feedback?(mh+mozilla) → feedback+

Comment 22

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> Untested yet. I suspect CPU/memory will be somewhat worse due to all
> conversions, but there should be less system calls, which we consider more
> important atm.
>  

I expect this to be faster because we do a lot of conversions in xpconnect.
(In reply to Taras Glek (:taras) from comment #22)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> > Untested yet. I suspect CPU/memory will be somewhat worse due to all
> > conversions, but there should be less system calls, which we consider more
> > important atm.
> >  
> 
> I expect this to be faster because we do a lot of conversions in xpconnect.

There is only one way to settle this: Benchmark Kombat!

Comment 24

5 years ago
Comment on attachment 609850 [details] [diff] [review]
Unix synchronous back-end

Please uses spaces around your operators.

I'm not sure what the benefit of wrapping FILE* functions is. I think wrapping basic fd-functions is sufficient for a low level api.
Attachment #609850 - Flags: feedback-

Comment 25

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #23)
> (In reply to Taras Glek (:taras) from comment #22)
> > (In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> > > Untested yet. I suspect CPU/memory will be somewhat worse due to all
> > > conversions, but there should be less system calls, which we consider more
> > > important atm.
> > >  
> > 
> > I expect this to be faster because we do a lot of conversions in xpconnect.
> 
> There is only one way to settle this: Benchmark Kombat!

It's true. Another thing to keep in mind that JIT guys have a hope in hell in optimizing ctypes code at runtime, not such hopes for layering-heavy xpconnect :)

Comment 26

5 years ago
(In reply to Taras Glek (:taras) from comment #24)
> Comment on attachment 609850 [details] [diff] [review]
> Unix synchronous back-end
> 
> Please uses spaces around your operators.
> 
> I'm not sure what the benefit of wrapping FILE* functions is. I think
> wrapping basic fd-functions is sufficient for a low level api.

I should clarify this. I think we should add functions as we need them instead of aggressively mirroring libc from getgo. The big benefit of an extensible ctypes-based api like this is that if we miss an api that a user might need, they can add it themselves.
Created attachment 612541 [details] [diff] [review]
Unix synchronous back-end

Ok, I have removed some functions that are probably not going to be useful in the near future, fixed a few trivial issues, added a little documentation, removed |gDeclareType| et al and resolved the scope of |export| (thanks, ochameau).
Attachment #609850 - Attachment is obsolete: true
Attachment #612541 - Flags: review?(taras.mozilla)
Created attachment 612543 [details] [diff] [review]
Companion testsuite

Extended the test suite, reworked it a little so that it has chances of working with Android. Additional tests: access, read, write, unlink.
Attachment #609852 - Attachment is obsolete: true
Attachment #612543 - Flags: review?(taras.mozilla)
Created attachment 612546 [details] [diff] [review]
Companion makefile
Attachment #612546 - Flags: review?(taras.mozilla)
Created attachment 612560 [details] [diff] [review]
Companion testsuite
Attachment #612543 - Attachment is obsolete: true
Attachment #612560 - Flags: review?(taras.mozilla)
Attachment #612543 - Flags: review?(taras.mozilla)

Comment 31

5 years ago
Comment on attachment 612541 [details] [diff] [review]
Unix synchronous back-end

r+, but please get rid of *at functions until we need them.

A real tookit reviewer needs to review this before this lands.
Attachment #612541 - Flags: review?(taras.mozilla)
Attachment #612541 - Flags: review?(doug.turner)
Attachment #612541 - Flags: review+

Comment 32

5 years ago
Comment on attachment 612560 [details] [diff] [review]
Companion testsuite

looks good
Attachment #612560 - Flags: review?(taras.mozilla) → review+

Comment 33

5 years ago
Comment on attachment 612546 [details] [diff] [review]
Companion makefile

I don't review makefiles unless I wrote them
Attachment #612546 - Flags: review?(taras.mozilla)
Created attachment 612832 [details] [diff] [review]
Unix synchronous back-end
Attachment #612541 - Attachment is obsolete: true
Attachment #612832 - Flags: review?(doug.turner)
Attachment #612541 - Flags: review?(doug.turner)
Created attachment 612837 [details] [diff] [review]
Non-Unix specific functions of the back-end

Ok, I have removed *at, as requested by Taras, and I have taken the opportunity to divide the patch in two. First part of the patch contains the Unix specific bits, while the second part of the patch contains bits that are shared between Unix and Windows implem.
Attachment #612837 - Flags: review?(taras.mozilla)
Attachment #612837 - Flags: review?(doug.turner)
Attachment #612832 - Flags: review?(taras.mozilla)

Comment 36

5 years ago
Comment on attachment 612832 [details] [diff] [review]
Unix synchronous back-end

+     let libc_candidates =  ["libSystem.dylib",
+                             "libc.so.6",
+                             "libc.so"];
+     for (let i = 0; i < libc_candidates.length; ++i) {

use for each

I think it's better to use todo instead of "note".
Attachment #612832 - Flags: review?(taras.mozilla) → review+

Comment 37

5 years ago
Comment on attachment 612837 [details] [diff] [review]
Non-Unix specific functions of the back-end

this seems ok. 

Nit:
+       let key = ":"+type.name;
put spaces around your operators, elsewhere too.
Attachment #612837 - Flags: review?(taras.mozilla) → review+
Comment on attachment 612837 [details] [diff] [review]
Non-Unix specific functions of the back-end

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

fixed up the ws on checkin.  r+ if you have answers for my questions.

::: toolkit/components/osfile/osfile_shared.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +{
> +  if (typeof Components != "undefined") {

Is this really the right way to test that we are running this jsm on the right thread?

@@ +11,5 @@
> +
> +    throw new Error("osfile_shared.jsm cannot be used from the main thread yet");
> +  }
> +
> +  (function(exports) {

Why not do something like:

if (exports.OS)
  return;

exports.OS = {};
exports.OS.Shared = {}
...

@@ +64,5 @@
> +       this.name = name;
> +       this.implementation = implementation;
> +       if (convert_from_c) {
> +         this.convert_from_c = convert_from_c;
> +       } else {// Optimization: Ensure harmony of shapes

what is harmony of shapes?

@@ +65,5 @@
> +       this.implementation = implementation;
> +       if (convert_from_c) {
> +         this.convert_from_c = convert_from_c;
> +       } else {// Optimization: Ensure harmony of shapes
> +         this.convert_from_c = Type.prototype.convert_from_c;

this.convert_from_c = convert_from_c || Type.prototype.convert_from_c; ?

@@ +91,5 @@
> +      */
> +     Types.int =
> +       new Type("int",
> +                ctypes.int);
> +     Types.int32_t =

missing white space after the "int" type

@@ +110,5 @@
> +                ctypes.int);
> +     Types.off_t =
> +       new Type("off_t",
> +                ctypes.int);
> +     Types.size_t =

Missing white space after the off_t type

@@ +113,5 @@
> +                ctypes.int);
> +     Types.size_t =
> +       new Type("size_t",
> +                ctypes.size_t);
> +     Types.ssize_t =

same

@@ +116,5 @@
> +                ctypes.size_t);
> +     Types.ssize_t =
> +       new Type("ssize_t",
> +                ctypes.ssize_t);
> +     Types.DWORD =

same

@@ +185,5 @@
> +      */// Note: Future versions will use a different implementation of this
> +        // function on the main thread, osfile worker thread and regular worker
> +        // thread
> +     let declareFFI = function declareFFI(lib, symbol, abi,
> +                                           returnType /*, argTypes ...*/) {

another ws nit.  line up returnType with lib

@@ +190,5 @@
> +       LOG("Attempting to declare FFI ", symbol);
> +       // We guard agressively, to avoid any late surprise
> +       if (typeof symbol != "string") {
> +         if (symbol instanceof String) {
> +           symbol = symbol.toString();

is there ever a |symbol| that is a typeof string, but isn't a instance of String?
Attachment #612837 - Flags: review?(doug.turner) → review+

Updated

5 years ago
Attachment #612832 - Flags: review?(doug.turner) → review+
(In reply to Doug Turner (:dougt) from comment #38)
> Comment on attachment 612837 [details] [diff] [review]
> Non-Unix specific functions of the back-end
> 
> Review of attachment 612837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> fixed up the ws on checkin.  r+ if you have answers for my questions.
> 
> ::: toolkit/components/osfile/osfile_shared.jsm
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +{
> > +  if (typeof Components != "undefined") {
> 
> Is this really the right way to test that we are running this jsm on the
> right thread?

This is the only technique I know of, but I can investigate further with the JS team.

> > +  (function(exports) {
> 
> Why not do something like:
> 
> if (exports.OS)
>   return;
> 
> exports.OS = {};
> exports.OS.Shared = {}

Not 100% sure I understand your question. I am actually building |OS.| in several layers, so I need some more careful checking.

> what is harmony of shapes?

I want to ensure that the VM representation  of the object always has the exact same Shape (as in "these fields are defined in the immediate object, not in a prototype"). I have not benchmarked this, so this may well be premature optimization.

> 
> @@ +65,5 @@
> > +       this.implementation = implementation;
> > +       if (convert_from_c) {
> > +         this.convert_from_c = convert_from_c;
> > +       } else {// Optimization: Ensure harmony of shapes
> > +         this.convert_from_c = Type.prototype.convert_from_c;
> 
> this.convert_from_c = convert_from_c || Type.prototype.convert_from_c; ?

ok.

> 
> @@ +91,5 @@
> > +      */
> > +     Types.int =
> > +       new Type("int",
> > +                ctypes.int);
> > +     Types.int32_t =
> 
> missing white space after the "int" type

Not sure I understand. Do you want a new line?

> > +      */// Note: Future versions will use a different implementation of this
> > +        // function on the main thread, osfile worker thread and regular worker
> > +        // thread
> > +     let declareFFI = function declareFFI(lib, symbol, abi,
> > +                                           returnType /*, argTypes ...*/) {
> 
> another ws nit.  line up returnType with lib
ok

> 
> @@ +190,5 @@
> > +       LOG("Attempting to declare FFI ", symbol);
> > +       // We guard agressively, to avoid any late surprise
> > +       if (typeof symbol != "string") {
> > +         if (symbol instanceof String) {
> > +           symbol = symbol.toString();
> 
> is there ever a |symbol| that is a typeof string, but isn't a instance of
> String?

I had the case in a previous version of this code, but I probably do not need that check anymore. Do you want me to remove it?
> Not sure I understand. Do you want a new line?

Yes, basically if you look at all of those assignments, most have one line of white space between them.  Just a nit for us to be consistent.


> Do you want me to remove it?

Yes, or add a comment as to why this case could exist.

Otherwise great stuff!
Thanks.

By the way, I have had confirmation from Jorendorff (and #developers silence): detecting the presence of |Components| seems to be the state of the art for detecting whether JS code is executed in the main thread or in a worker.
Created attachment 617436 [details] [diff] [review]
Non-Unix specific functions of the back-end
Attachment #612837 - Attachment is obsolete: true
Created attachment 617437 [details] [diff] [review]
Unix synchronous back-end
Attachment #612832 - Attachment is obsolete: true
Created attachment 617438 [details] [diff] [review]
Companion testsuite
Attachment #612560 - Attachment is obsolete: true
(In reply to Taras Glek (:taras) from comment #36)
> Comment on attachment 612832 [details] [diff] [review]
> Unix synchronous back-end
> 
> +     let libc_candidates =  ["libSystem.dylib",
> +                             "libc.so.6",
> +                             "libc.so"];
> +     for (let i = 0; i < libc_candidates.length; ++i) {
> 
> use for each

In another bug, Gavin recommended not using for each on arrays, as this can be fooled if the array has additional properties. While this is quite unlikely in a JS module, chrome workers make no such guarantee, so I would rather not take the chance.

> I think it's better to use todo instead of "note".

Well, the note is actually more an explanation of why things are designed like this, rather than a TODO.
Blocks: 747876
Created attachment 618690 [details] [diff] [review]
Unix synchronous back-end

A few minor updates to the various patches, reflecting current work on the front-end. I promise I will now stop updating anything other than bugs :)
Attachment #617437 - Attachment is obsolete: true
Attachment #618690 - Flags: review?(doug.turner)

Comment 47

5 years ago
Comment on attachment 618690 [details] [diff] [review]
Unix synchronous back-end

+       UnixFile.close = function(fd) {
+         fd.dispose();
+         if (ctypes.errno) {
+           throw new OSUnix.Error(ctypes.errno, "close");
+         }
+       };
+

same as in the windows bug, please do not modify semantics of low level functions, instead directly call close(fd) and bypass the finalizer. (Sorry if I missed this before)
Attachment #618690 - Flags: review?(doug.turner) → review-
(In reply to Taras Glek (:taras) from comment #47)
> Comment on attachment 618690 [details] [diff] [review]
> Unix synchronous back-end
> 
> +       UnixFile.close = function(fd) {
> +         fd.dispose();
> +         if (ctypes.errno) {
> +           throw new OSUnix.Error(ctypes.errno, "close");
> +         }
> +       };
> +
> 
> same as in the windows bug, please do not modify semantics of low level
> functions, instead directly call close(fd) and bypass the finalizer. (Sorry
> if I missed this before)

Ok, I will remove all the exceptions everywhere. Can I at least return an error data structure instead of -1 / 0 / NULL / INVALID_HANDLE / ...?
Created attachment 618985 [details] [diff] [review]
Unix synchronous back-end

Same code, without exceptions and with simpler |close|.
Attachment #618690 - Attachment is obsolete: true
Attachment #618985 - Flags: review?(taras.mozilla)
Created attachment 618986 [details] [diff] [review]
Non-Unix specific functions of the back-end
Attachment #617436 - Attachment is obsolete: true
Attachment #618986 - Flags: review?(taras.mozilla)
Created attachment 618987 [details] [diff] [review]
Companion testsuite

Propagated the changes to the test suite.
Attachment #617438 - Attachment is obsolete: true
Attachment #618987 - Flags: review?(taras.mozilla)
Created attachment 619056 [details] [diff] [review]
Unix synchronous back-end

Same patch with a little more polish. Added a hack to ensure that we can check whether |open| returns -1, removed redundant |_strerror| and |OS.Unix.Error|.
Attachment #618985 - Attachment is obsolete: true
Attachment #619056 - Flags: review?(taras.mozilla)
Attachment #618985 - Flags: review?(taras.mozilla)

Comment 53

5 years ago
Comment on attachment 619056 [details] [diff] [review]
Unix synchronous back-end

+       UnixFile.close = function close(fd) {
+         return fd.dispose();
+       };

I think should should say

return _close(fd.forget()).
Attachment #619056 - Flags: review?(taras.mozilla)

Comment 54

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #48)

> Ok, I will remove all the exceptions everywhere. Can I at least return an
> error data structure instead of -1 / 0 / NULL / INVALID_HANDLE / ...?

I think wrappers should be wrappers, we can introduce diversions from the platform at higher levels.

Updated

5 years ago
Attachment #618987 - Flags: review?(taras.mozilla) → review+

Comment 55

5 years ago
Comment on attachment 618986 [details] [diff] [review]
Non-Unix specific functions of the back-end

I think that's ok.
Attachment #618986 - Flags: review?(taras.mozilla) → review+
(In reply to Taras Glek (:taras) from comment #53)
> Comment on attachment 619056 [details] [diff] [review]
> Unix synchronous back-end
> 
> +       UnixFile.close = function close(fd) {
> +         return fd.dispose();
> +       };
> 
> I think should should say
> 
> return _close(fd.forget()).

Well, both have the exact same semantics, but I would prefer keeping the |dispose| version, if you do not mind, as it is faster, shorter and (I believe) simpler.
Created attachment 619541 [details] [diff] [review]
Unix synchronous back-end

Minor fix wrt the return of |OS.File.Unix.pipe| in case of error. Also, see my comment above regarding |dispose|.
Attachment #619056 - Attachment is obsolete: true
Attachment #619541 - Flags: review?(taras.mozilla)

Comment 58

5 years ago
Comment on attachment 619541 [details] [diff] [review]
Unix synchronous back-end

+
+       UnixFile.pipe = function pipe(array) {
+         if (_pipe(_pipebuf) == -1) {
+           return -1;
+         }
+         array[0] = ctypes.CDataFinalizer(_pipebuf[0], _close);
+         array[1] = ctypes.CDataFinalizer(_pipebuf[1], _close);
+         return 0;
+       };

nit: I would prefer to do ret = _pipe...; if (ret == -1) return ret; ..... return ret. 

You can keep your dispose call, but please comment that you are relying on the finalizer calling close and dispose returning proper success/error code
Attachment #619541 - Flags: review?(taras.mozilla) → review+
Created attachment 621519 [details] [diff] [review]
Unix synchronous back-end
Attachment #619541 - Attachment is obsolete: true
Created attachment 623149 [details] [diff] [review]
Companion makefile
Attachment #612546 - Attachment is obsolete: true
Created attachment 623151 [details] [diff] [review]
Companion makefile
Attachment #623149 - Attachment is obsolete: true
Attachment #621519 - Flags: review?(doug.turner)

Updated

5 years ago
Attachment #621519 - Flags: review?(doug.turner) → review+
Created attachment 625591 [details] [diff] [review]
Companion makefile
Attachment #623151 - Attachment is obsolete: true
Created attachment 625592 [details] [diff] [review]
Companion testsuite
Attachment #618987 - Attachment is obsolete: true
Created attachment 625593 [details] [diff] [review]
Unix synchronous back-end

Fixed a discrepancy between Linux and BSD (behavior of |getwd| is slightly different).
Attachment #621519 - Attachment is obsolete: true
Created attachment 625594 [details] [diff] [review]
Non-Unix specific functions of the back-end

Fixed an issue with 32-bits vs. 64-bits.
Attachment #618986 - Attachment is obsolete: true
Created attachment 625597 [details] [diff] [review]
Non-Unix specific functions of the back-end
Depends on: 757469
Created attachment 626428 [details] [diff] [review]
Non-Unix specific functions of the back-end

Fixed a bug that caused off_t to have the wrong size on some configurations.
Attachment #625594 - Attachment is obsolete: true
Attachment #625597 - Attachment is obsolete: true
Created attachment 626429 [details] [diff] [review]
Companion testsuite
Attachment #625592 - Attachment is obsolete: true
Created attachment 626433 [details] [diff] [review]
Companion makefile
Attachment #625591 - Attachment is obsolete: true
Created attachment 626434 [details] [diff] [review]
Unix synchronous back-end
Attachment #625593 - Attachment is obsolete: true
Created attachment 626436 [details] [diff] [review]
Companion makefile
Attachment #626433 - Attachment is obsolete: true
Created attachment 627461 [details] [diff] [review]
Companion makefile
Attachment #626436 - Attachment is obsolete: true
Created attachment 627870 [details] [diff] [review]
Companion testsuite
Attachment #626429 - Attachment is obsolete: true
Created attachment 627871 [details] [diff] [review]
Unix synchronous back-end
Attachment #626434 - Attachment is obsolete: true
Created attachment 627872 [details] [diff] [review]
Non-Unix specific functions of the back-end
Attachment #626428 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 707681
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bb11da7be6
https://hg.mozilla.org/integration/mozilla-inbound/rev/378acab7d10c
https://hg.mozilla.org/integration/mozilla-inbound/rev/87174904a926
https://hg.mozilla.org/integration/mozilla-inbound/rev/43b7500f5954

Sorry for the delay...
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/f7bb11da7be6
https://hg.mozilla.org/mozilla-central/rev/378acab7d10c
https://hg.mozilla.org/mozilla-central/rev/87174904a926
https://hg.mozilla.org/mozilla-central/rev/43b7500f5954
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 785674
Depends on: 786860
You need to log in before you can comment on or make changes to this bug.