Closed Bug 707909 Opened 9 years ago Closed 8 years ago

[OS.File] serialize files

Categories

(Toolkit :: OS.File, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: main-thread-io, perf)

Attachments

(1 file, 1 obsolete file)

Sending OS.File, OS.Dir objects to a worker would be very useful. We should implement this.

It is probably quite easy to do the following:
- add a factory OS.File.ofHandle;
- add a custom toSource() for OS.File, OS.Dir that returns respectively "new OS.File.ofHandle(...)" and "new OS.Dir(...)".

Is this sufficient?
Assignee: dteller → nobody
Severity: normal → enhancement
Component: JavaScript Engine → Networking: File
QA Contact: general → networking.file
Summary: Efficient JS File API - (de)serialize files and directories → [OS.File] (de)serialize files and directories
After experimenting, we need something a little different. Files are opened in a I/O worker thread, then sent to the main thread. Garbage-collection, however, should apply only to the main thread.

So, we need the following components:
- in the worker-only implementation of OS.File, an operation |OS.File.prototype.forget| that ensures that garbage-collection does not close the file;
- in the worker-only implementation of OS.File, an operation |OS.File.toMsg| that returns the file descriptor in a form that can be sent to the main thread;
- in the worker-only implementation of OS.File, a type |OS.Shared.Type.file_handle| that can be used to deserialize this descriptor;
- main thread stuff (in another bug).

Same stuff for directory iterators.
Blocks: MTOS.File
No longer blocks: OS.File
Summary: [OS.File] (de)serialize files and directories → [OS.File] serialize files and directories
Re-scoping: let's handle files for the moment, directories later.
Assignee: nobody → dteller
Component: Networking: File → OS.File
Product: Core → Toolkit
Summary: [OS.File] serialize files and directories → [OS.File] serialize files
This patch implements supports for serialization of files.
Deserialization support will be distinct between main thread and worker threads, due to garbage-collection issues.
Attachment #649557 - Flags: review?(nfroyd)
Comment on attachment 649557 [details] [diff] [review]
Support code for (de)serializing files

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

Clearing r? due to wrong patch.
Attachment #649557 - Flags: review?(nfroyd)
Oops
Attachment #649557 - Attachment is obsolete: true
Attachment #649668 - Flags: review?(nfroyd)
Attachment #649668 - Attachment is patch: true
Comment on attachment 649668 [details] [diff] [review]
Support code for (de)serializing files

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

I don't see a definition of fromMsg anywhere, which is what I would expect to see _setHandle used for.  Am I missing something, or is the patch incomplete?
The definitions of fromMsg (one for the controller, one for the worker) are part of bug 777711 and I have not managed to detach them cleanly.

Indeed, the worker code uses |_setHandle|. The controller code... is a little more imaginative :)
Comment on attachment 649668 [details] [diff] [review]
Support code for (de)serializing files

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

This is a little tricky to review since it's at best half of the story (no deserialization, no tests).  Let me know if I'm wildly off base with my suggestions and if I am, I'll try to take a better look at the other bits in bug 777711.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +107,5 @@
> +  _setHandle: function _setHandle(fd) {
> +    if (fd == null) {
> +      throw new TypeError("Expecting a non-null file handle");
> +    }
> +    this._fd = fd;

Seems like we'd want to check that this._fd is null here or try to clean up an existing _fd somehow; we'd leak in the latter case.

@@ +119,5 @@
> +  forget: function forget() {
> +    if (!this._fd) {
> +      throw new TypeError("Cannot call |forget| on a closed File");
> +    }
> +    this._fd.forget();

Seems like you'd want to return this value and/or null out this._fd.

@@ +128,5 @@
> + * Convert an instance of |File| into something that can be serialized
> + * between threads (not necessarily a string).
> + *
> + * Note: If necessary, do not forget to call
> + * |File.prototype.forget| to ensure that your file descriptor

.prototype makes my head hurt sometimes; don't you mean "...do not forget to call |forget| to ensure..."?

@@ +138,5 @@
> +    return null;
> +  }
> +  // |fd| may or may not be a CDataFinalizer, get back to the C type
> +  let as_cval = exports.OS.Shared.Type.fd.implementation(fd);
> +  let as_msg = exports.OS.Shared.Type.fd.toMsg(as_cval);

It surprises me that with all the emphasis on types that you have, we're simply relying on the serialization of as_cval here, rather than packaging it up into an { fd: ... } object.

@@ +139,5 @@
> +  }
> +  // |fd| may or may not be a CDataFinalizer, get back to the C type
> +  let as_cval = exports.OS.Shared.Type.fd.implementation(fd);
> +  let as_msg = exports.OS.Shared.Type.fd.toMsg(as_cval);
> +  return as_msg;

Nit: just dispense with as_msg here.
Attachment #649668 - Flags: review?(nfroyd) → review-
FYI, Taras wants me to change a big piece of the design. There are chances that this will make this bug useless.
Indeed, this has become useless atm.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.