Port async file operations to be using native ports instead or an isolate

Port async file operations to be using native ports instead or an isolate

R=ager@google.com

Committed: https://code.google.com/p/dart/source/detail?r=4520

[Søren Gjesse, 2012-02-17 15:12:32]

This is still a but rough around the edges, but it implements all operations.

[Mads Ager (google), 2012-02-20 13:50:31]

It is great that we are getting all of this offloaded to the thread pool!

There are a couple of TODOs that it seems to me we should deal with before
committing this.

Using list literals should make the argument packing nicer I think?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.cc#...
runtime/bin/file.cc:363: T* CObjectToPointer(CObject* cobject) {
Can't we just have a:

static File* CObjectToFilePointer(CObject* cobject)

I don't think we use it for other types in any case (and if we did maybe it
should be in dartutils?).

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.cc#...
runtime/bin/file.cc:548:
file->Read(reinterpret_cast<void*>(byte_array->Buffer()),
It might be more readable to do the casting outside before the call?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.cc#...
runtime/bin/file.cc:580: // TODO(sgjesse): Return error.
Is this new or did we handle this the same way before?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.cc#...
runtime/bin/file.cc:602: file->Write(reinterpret_cast<const
void*>(str.CString()),
Might be more readable to do the casting on a line before?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.cc#...
runtime/bin/file.cc:682: service_port =  Dart_NewNativePort("FileService",
Are there two spaces after '=' here?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.h
File runtime/bin/file.h (right):

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file.h#n...
runtime/bin/file.h:122: static FileOpenMode DartModeToFileMode(int mode);
Should the type here be DartFileOpenMode? Should this be private including the
enums above?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file_imp...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file_imp...
runtime/bin/file_impl.dart:211: if (_fileService == null) {
How about having an ensureFileService here as well?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file_imp...
runtime/bin/file_impl.dart:221: List request = new List(2);
Why not use list literals here?

var request = [ _FileUtils.kExistsRequest, _name ];

I think that would be a lot more readable.

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file_imp...
runtime/bin/file_impl.dart:225: if (_existsHandler != null)
_existsHandler(exists);
Indentation is off. Here and in the rest of the file.

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file_imp...
runtime/bin/file_impl.dart:562: // TODO(sgjesse): Handle no pending writes
correctly.
This one looks important. This should be fixed before committing?

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/io.dart
File runtime/bin/io.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/io.dart#...
runtime/bin/io.dart:12: #import("dart:builtin");
Do you need this or was that for debug printing?

https://chromiumcodereview.appspot.com/9415043/diff/2001/tests/standalone/src...
File tests/standalone/src/FileTest.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/2001/tests/standalone/src...
tests/standalone/src/FileTest.dart:470: // TODO(sgjesse): Handle this.
Should be handled before commit as well.

[Søren Gjesse, 2012-02-21 14:22:38]

Still missing

 1. Handling of writeList with non integers in the list (how about bigints ?)
 2. Handling on noPendingWrites
 3. Handling of all types of lists (also List literals and user-implemented
Lists)

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.cc#newcode363
runtime/bin/file.cc:363: T* CObjectToPointer(CObject* cobject) {
On 2012/02/20 13:50:31, Mads Ager wrote:
> Can't we just have a:
> 
> static File* CObjectToFilePointer(CObject* cobject)
> 
> I don't think we use it for other types in any case (and if we did maybe it
> should be in dartutils?).

Done.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.cc#newcode548
runtime/bin/file.cc:548:
file->Read(reinterpret_cast<void*>(byte_array->Buffer()),
On 2012/02/20 13:50:31, Mads Ager wrote:
> It might be more readable to do the casting outside before the call?

Done.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.cc#newcode580
runtime/bin/file.cc:580: // TODO(sgjesse): Return error.
On 2012/02/20 13:50:31, Mads Ager wrote:
> Is this new or did we handle this the same way before?

Before we used Dart_ListGetAsBytes and if there is a non integer in the list an
error is raised. In File_WriteList we have an "ASSERT(!Dart_IsError(result))" -
not that good. We should return an error here.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.cc#newcode602
runtime/bin/file.cc:602: file->Write(reinterpret_cast<const
void*>(str.CString()),
On 2012/02/20 13:50:31, Mads Ager wrote:
> Might be more readable to do the casting on a line before?

Done.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.cc#newcode682
runtime/bin/file.cc:682: service_port =  Dart_NewNativePort("FileService",
On 2012/02/20 13:50:31, Mads Ager wrote:
> Are there two spaces after '=' here?

Yes, removed one.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.h
File runtime/bin/file.h (right):

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file.h#newcode122
runtime/bin/file.h:122: static FileOpenMode DartModeToFileMode(int mode);
On 2012/02/20 13:50:31, Mads Ager wrote:
> Should the type here be DartFileOpenMode? Should this be private including the
> enums above?

Changed the type. Cannot be private as it is used by the File_Open "native"
function.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:211: if (_fileService == null) {
On 2012/02/20 13:50:31, Mads Ager wrote:
> How about having an ensureFileService here as well?

Done.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:221: List request = new List(2);
On 2012/02/20 13:50:31, Mads Ager wrote:
> Why not use list literals here?
> 
> var request = [ _FileUtils.kExistsRequest, _name ];
> 
> I think that would be a lot more readable.

Yes, but we cannot serialize list literals :-( I will fix that in a separate
change before committing this, as other parts of this change will depend on that
anyway.

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:225: if (_existsHandler != null)
_existsHandler(exists);
On 2012/02/20 13:50:31, Mads Ager wrote:
> Indentation is off. Here and in the rest of the file.

Done (Emacs Dart mode thinks this is the way).

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/io.dart
File runtime/bin/io.dart (right):

http://codereview.chromium.org/9415043/diff/2001/runtime/bin/io.dart#newcode12
runtime/bin/io.dart:12: #import("dart:builtin");
On 2012/02/20 13:50:31, Mads Ager wrote:
> Do you need this or was that for debug printing?

Reverted.

[Søren Gjesse, 2012-02-22 16:27:07]

Ass issues/comments have now been addressed, including:

 1. Handling of writeList with non integers in the list (how about bigints ?)
 2. Handling of noPendingWrites
 3. Handling of all types of lists (also List literals and user-implemented
Lists)

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file_imp...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/2001/runtime/bin/file_imp...
runtime/bin/file_impl.dart:562: // TODO(sgjesse): Handle no pending writes
correctly.
On 2012/02/20 13:50:31, Mads Ager wrote:
> This one looks important. This should be fixed before committing?

Done.

https://chromiumcodereview.appspot.com/9415043/diff/2001/tests/standalone/src...
File tests/standalone/src/FileTest.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/2001/tests/standalone/src...
tests/standalone/src/FileTest.dart:470: // TODO(sgjesse): Handle this.
On 2012/02/20 13:50:31, Mads Ager wrote:
> Should be handled before commit as well.

Done.

[Mads Ager (google), 2012-02-23 08:57:44]

LGTM!

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file.cc...
runtime/bin/file.cc:62: dart_file_mode = static_cast<File::DartFileOpenMode>(
How about extracting the int first?

int mode = DartUtils::GetIntegerValue(Dart_GetNativeArgument(args, 1));
File::DartFileOpenMode dart_file_mode =
    static_cast<File::DartFileOpenMode>(mode);

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file_im...
runtime/bin/file_impl.dart:178: static List ensureFastAndSerializabelBuffer(
Serializable

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file_im...
runtime/bin/file_impl.dart:965: void  _writesCompleted() {
writes -> write

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/io.dart
File runtime/bin/io.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/io.dart...
runtime/bin/io.dart:12: #import("dart:builtin");
Remove.

https://chromiumcodereview.appspot.com/9415043/diff/13009/tests/standalone/sr...
File tests/standalone/src/FileTest.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/tests/standalone/sr...
tests/standalone/src/FileTest.dart:426: openedFile.close();
Why did you need to rewrite this test?

[Søren Gjesse, 2012-02-23 10:26:42]

Also ported directory() - do you want to check that as well

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file.cc...
runtime/bin/file.cc:62: dart_file_mode = static_cast<File::DartFileOpenMode>(
On 2012/02/23 08:57:44, Mads Ager wrote:
> How about extracting the int first?
> 
> int mode = DartUtils::GetIntegerValue(Dart_GetNativeArgument(args, 1));
> File::DartFileOpenMode dart_file_mode =
>     static_cast<File::DartFileOpenMode>(mode);

Done.

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file_im...
runtime/bin/file_impl.dart:178: static List ensureFastAndSerializabelBuffer(
On 2012/02/23 08:57:44, Mads Ager wrote:
> Serializable

Done.

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/file_im...
runtime/bin/file_impl.dart:965: void  _writesCompleted() {
On 2012/02/23 08:57:44, Mads Ager wrote:
> writes -> write

Done.

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/io.dart
File runtime/bin/io.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/runtime/bin/io.dart...
runtime/bin/io.dart:12: #import("dart:builtin");
On 2012/02/23 08:57:44, Mads Ager wrote:
> Remove.

Done.

https://chromiumcodereview.appspot.com/9415043/diff/13009/tests/standalone/sr...
File tests/standalone/src/FileTest.dart (right):

https://chromiumcodereview.appspot.com/9415043/diff/13009/tests/standalone/sr...
tests/standalone/src/FileTest.dart:426: openedFile.close();
On 2012/02/23 08:57:44, Mads Ager wrote:
> Why did you need to rewrite this test?

Actually I didn't - reverted (I was testing calling close directly after write -
we should add some tests for that).

[Mads Ager (google), 2012-02-23 11:01:47]

lgtm