707676 - Efficient JS File API - JSAPI binding

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

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

Expose the platform-specific backends of the efficient JS File API to JavaScript. For performance, predictability and garbage-collection issues, this binding is based on JSAPI.

Comment 1

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

Attachment: Prev 2. JSAPI binding

Comment 2

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

Attachment: Prev 3. JSAPI bindings

New version of the JSAPI bindings. Cleaned-up, hardened, and with initial support for _OSDir.

Comment 3

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

Attachment: Exporting the bindings to various contexts.

Comment 4

[David Mandelin [:dmandelin]]

Comment on attachment 581679 [details] [diff] [review]
Prev 3. JSAPI bindings

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

Mostly reviewing just the JSAPI aspects. I see mostly minor issues now--generally stuff where there's probably a slightly simpler way of doing it. I do still have some questions about the string buffer setup. There's also a fair amount of duplicated code, so that should probably be cleaned up too.

::: toolkit/components/osfile/RawFileService.cpp
@@ +51,5 @@
> +
> +typedef mozilla::rawfile::RawDirectory RawDirectory;
> +typedef mozilla::rawfile::RawFile      RawFile;
> +
> +/**

The comment seems to be inaccurate. By the way, my personal taste is not to use macros like this, because "if (!x) return false" is simple and idiomatic enough. But, I don't object to your using it in this file if you find it useful.

@@ +100,5 @@
> +/**
> + * Utility function for setting a Number property from a Uint32.
> + */
> +JSBool SetUint32Property(JSContext *aCx, JSObject *aObject,
> +                         const char *aProperty, PRUint32 aValue)

We use JSBool in jsapi.h for C compatibility. We just use bool internally, so I recommend that you do that too.

@@ +102,5 @@
> + */
> +JSBool SetUint32Property(JSContext *aCx, JSObject *aObject,
> +                         const char *aProperty, PRUint32 aValue)
> +{
> +  jsval valValue;

Just use

jsval valValue = INT_TO_JSVAL(aValue);

@@ +106,5 @@
> +  jsval valValue;
> +  if (!JS_NewNumberValue(aCx, aValue, &valValue)) {
> +    return JS_FALSE;
> +  }
> +  if (!JS_SetProperty(aCx, aObject, aProperty, &valValue)) {

We'd write this

  return JS_SetProperty(aCx, aObject, aProperty, &valValue);

Same in the other helpers.

@@ +118,5 @@
> + */
> +JSBool SetDateProperty(JSContext *aCx, JSObject *aObject, const char *aProperty,
> +                       PRTime aTime)
> +{
> +  JSObject *objTime = JS_NewDateObjectMsec(aCx, ((jsdouble)aTime)/1000000);

jsdouble is some old-school thing that's not needed anymore, just use double.

@@ +229,5 @@
> + *             The file descriptor/handle
> + */
> +//FIXME: For the moment, we assume that a handle is 32 bits.
> +//This is not true under Windows. A better solution would
> +//certainly be to return a Uint8array.

If all you care about is being able to store a handle and interact with it in C++, you could use the RESERVED_SLOTS feature, where you define some reserved slots in the class definition, and then use JS_(Get|Set)ReservedSlot. In ES6, you'll probably be able to use the binary data API too.

@@ +267,5 @@
> + * - unixLastStatusChangeDate
> + * - blockSize
> + *
> + * All the fields that do not appear in this list are ignored.
> + */

IIUC, the API of this function is that you pass in an object with a bunch of properties, and those properties get replaced by the values from the file. That seems a little unusual to me. Is it that important to be able to fill in only some of the data? I'm not reviewing the API design itself, just wanted to note my reaction.

@@ +295,5 @@
> +    JS_SET_RVAL(aCx, aVp, JSVAL_VOID);
> +    return JS_TRUE;
> +  }
> +
> +  JSBool hasSize;

You could remove the redundancy here using the .tbl file trick that we use in SpiderMonkey.

@@ +360,5 @@
> +  //cached information if available.
> +
> +  JSBool requireUpdate = JS_TRUE;
> +  if (aArgc >= 2) {
> +    if(!JS_ValueToBoolean(aCx, argv[1], &requireUpdate)) {

JS_ValueToBoolean always returns JS_TRUE. If you want to require a boolean argument, use JSVAL_IS_BOOLEAN. But it seems pretty reasonable to me to just use the regular conversion process, in which case you'd just delete the error handling.

@@ +395,5 @@
> +    ReportError(aCx, "Could not fetch file info", rv);
> +    return JS_FALSE;
> +  }
> +
> +  //5. Fill the record

I'd suggest doing something to reduce the copied boilerplate here, but the usage of JSAPI looks good.

@@ +605,5 @@
> +  PRUint32 fileOffset = 0;
> +  PRUint32 bytesToRead = PR_UINT32_MAX;
> +  if (aArgc > 0) {
> +    JSObject *objPosition;
> +    if(!JS_ValueToObject(aCx, argv[0], &objPosition)) {

This will convert primitives to objects, so for your usage you probably want to test JSVAL_IS_OBJECT. It won't hurt anything though since the next lookup will just fail.

@@ +685,5 @@
> +
> +      break;
> +    }
> +  case BINARY_STRING:
> +    {//Binary string: simple memory copy + zero-terminate it

Just to make sure I understand the intent, I think this version is trying to create a JS string that contains (a zero-terminated byte string of the file contents). E.g., if the file contained "foo", the JS string would be two characters: one of 'f' and 'o' in arbitrary order, and one of 'o' and '\0' in the same order. If that's wrong, then my review comments will be wrong. :-)

@@ +687,5 @@
> +    }
> +  case BINARY_STRING:
> +    {//Binary string: simple memory copy + zero-terminate it
> +      char     *bufResult;
> +

IIRC, sizeof(char) == 1 by spec, so I'd leave that off. To me, the size seems like it should be

  (szActualRead + 1 + 1) / sizeof(jschar)

which is

  (bytes read + 1 byte null term + 1 to make us round up) / sizeof(jschar)

@@ +694,5 @@
> +
> +      bufResult = (char*)JS_malloc(aCx, size*sizeof(jschar));
> +      CHECK_NULL(bufResult);
> +
> +      memcpy(bufResult, bufRead, szActualRead);

Shouldn't this be just

  bufResult[size - 1] = 0

?

@@ +696,5 @@
> +      CHECK_NULL(bufResult);
> +
> +      memcpy(bufResult, bufRead, szActualRead);
> +      ((jschar*)bufResult)[size - 1 ] = jschar(0);
> +

And this would then use just size. Maybe we're using a different definition of size. Anyway, I'd recommend documenting the meaning of size precisely--IME this offset arithmetic gets confusing unless it's really clear what's meant.

@@ +710,5 @@
> +    }
> +  case ENCODED_STRING://Note: For the moment, we only implement ASCII
> +    {
> +      jschar     *bufResult;
> +

JS strings aren't null-terminated, so you don't need the +1 or the null.

@@ +795,5 @@
> +  jsval *argv = JS_ARGV(aCx, aVp);
> +
> +  //Extract arg 0 (ArrayBuffer => char*)
> +  JSObject *objArrayBuffer;
> +  if(!JS_ValueToObject(aCx, argv[0], &objArrayBuffer)) {

Same as before, probably want JSVAL_IS_OBJECT here.

@@ +809,5 @@
> +
> +  char *buf = (char*)JS_GetArrayBufferData(objArrayBuffer);
> +  PRUint32 bufLength  = JS_GetArrayBufferByteLength(objArrayBuffer);
> +
> +  //Extract arg 1 (optional position)

There's a little code duplication here with the other position extraction thing.

@@ +925,5 @@
> +  //Extract position
> +  bool absolutePosition = false;
> +  PRUint32 fileOffset = 0;
> +  PRUint32 bufOffset = 0;
> +  PRUint32 bytesToWrite = PR_UINT32_MAX;

This code seems to be duplicated in all the write functions.

@@ +1259,5 @@
> +
> +  {
> +    jsval valAccess;
> +    if (JS_GetProperty(aCx, objFlags, NAME_ACCESS, &valAccess)) {
> +      if (valAccess != JSVAL_VOID) {

The undefined check doesn't seem necessary.

@@ +1384,5 @@
> +  JSCLASS_NO_OPTIONAL_MEMBERS
> +}; //As above, we may wish to implement a (de)serializer
> +
> +
> +/**

Wrong name in comment. I assume this part isn't supposed to be done, though.

Comment 5

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

(In reply to David Mandelin from comment #4)
> Comment on attachment 581679 [details] [diff] [review]
> Prev 3. JSAPI bindings
> 
> Review of attachment 581679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly reviewing just the JSAPI aspects. I see mostly minor issues
> now--generally stuff where there's probably a slightly simpler way of doing
> it. I do still have some questions about the string buffer setup. There's
> also a fair amount of duplicated code, so that should probably be cleaned up
> too.

Yes, I have slowly started deduplicating code. I will try and finish this by next review.

> The comment seems to be inaccurate. By the way, my personal taste is not to
> use macros like this, because "if (!x) return false" is simple and idiomatic
> enough. But, I don't object to your using it in this file if you find it
> useful.

I personally miss exceptions, does it show? :)

[...] Plenty of recommendations [...]

> @@ +229,5 @@
> > + *             The file descriptor/handle
> > + */
> > +//FIXME: For the moment, we assume that a handle is 32 bits.
> > +//This is not true under Windows. A better solution would
> > +//certainly be to return a Uint8array.
> 
> If all you care about is being able to store a handle and interact with it
> in C++, you could use the RESERVED_SLOTS feature, where you define some
> reserved slots in the class definition, and then use
> JS_(Get|Set)ReservedSlot. In ES6, you'll probably be able to use the binary
> data API too.

The objective is twofold:
- I want to be able to somehow that data to worker threads;
- I want to be able to send it to ctypes functions, should anyone need to use ctypes functions that accept file descriptors/file handles.

How I can do either is rather unclear to me atm.

> IIUC, the API of this function is that you pass in an object with a bunch of
> properties, and those properties get replaced by the values from the file.
> That seems a little unusual to me. Is it that important to be able to fill
> in only some of the data? I'm not reviewing the API design itself, just
> wanted to note my reaction.

The idea is to be able to optimize fetching fstat-style data. Under Windows, emulating |fstat| is quite expensive as it requires around 4 system calls to different subsystems. Some system calls also seem to be overlapping and provide the same pieces of information. For this reason, we are attempting to find out exactly what the user wants to determine, to decide which system call to perform.

However, I consider that this is the worst part of our API atm. I am looking for saner alternatives.

> Just to make sure I understand the intent, I think this version is trying to
> create a JS string that contains (a zero-terminated byte string of the file
> contents). E.g., if the file contained "foo", the JS string would be two
> characters: one of 'f' and 'o' in arbitrary order, and one of 'o' and '\0'
> in the same order. If that's wrong, then my review comments will be wrong.
> :-)

That's exactly the idea: use the JS string as a dumb container that certainly does not contain anything human-readable. Probably not the best way to use strings, but FileReader/FileWriter seem to provide this feature, so might as well provide it at lower-level, too.


> @@ +687,5 @@
> > +    }
> > +  case BINARY_STRING:
> > +    {//Binary string: simple memory copy + zero-terminate it
> > +      char     *bufResult;
> > +
> 
> IIRC, sizeof(char) == 1 by spec, so I'd leave that off. To me, the size
> seems like it should be
> 
>   (szActualRead + 1 + 1) / sizeof(jschar)
> 
> which is
> 
>   (bytes read + 1 byte null term + 1 to make us round up) / sizeof(jschar)

Sounds true.

> 
> @@ +694,5 @@
> > +
> > +      bufResult = (char*)JS_malloc(aCx, size*sizeof(jschar));
> > +      CHECK_NULL(bufResult);
> > +
> > +      memcpy(bufResult, bufRead, szActualRead);
> 
> Shouldn't this be just
> 
>   bufResult[size - 1] = 0
> 
> ?

Generally, when it comes to promoting something from one byte to two, I prefer erring on the safe side.

> @@ +696,5 @@
> > +      CHECK_NULL(bufResult);
> > +
> > +      memcpy(bufResult, bufRead, szActualRead);
> > +      ((jschar*)bufResult)[size - 1 ] = jschar(0);
> > +
> 
> And this would then use just size. Maybe we're using a different definition
> of size. Anyway, I'd recommend documenting the meaning of size
> precisely--IME this offset arithmetic gets confusing unless it's really
> clear what's meant.

Good point.


> @@ +710,5 @@
> > +    }
> > +  case ENCODED_STRING://Note: For the moment, we only implement ASCII
> > +    {
> > +      jschar     *bufResult;
> > +
> 
> JS strings aren't null-terminated, so you don't need the +1 or the null.

Ok, I must have misunderstood the documentation.

> Wrong name in comment. I assume this part isn't supposed to be done, though.

Indeed, design of the lower-level stuff is still much too shaky.

Thanks for the quick review.

Comment 6

[David Mandelin [:dmandelin]]

Comment on attachment 581680 [details] [diff] [review]
Exporting the bindings to various contexts.

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

This patch looks OK to me but I'm not really a qualified reviewer here. I would suggest bent.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 581680 [details] [diff] [review]
Exporting the bindings to various contexts.

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

::: dom/Makefile.in
@@ +82,5 @@
>    workers \
>    $(NULL)
>  
> +LOCAL_INCLUDES = \
> +  $(DEPTH)/toolkit/components/osfile \

I don't think this is necessary. At least not as part of this patch.

::: dom/workers/WorkerScope.cpp
@@ +881,5 @@
>    if (worker->IsChromeWorker()) {
> +    if (!chromeworker::InitClass(aCx, global, workerProto, false)
> +       || !chromeworker::DefineChromeWorkerFunctions(aCx, global)
> +       || !::mozilla::rawfile::DefineChromeWorkerFunctions(aCx, global)
> +       ) {

Nit: Please keep || and the ){ on the preceding line.

::: toolkit/library/Makefile.in
@@ +61,5 @@
>  MAKE_FRAMEWORK=1
>  endif
>  
> +LOCAL_INCLUDES = \
> +  -I$(topsrcdir)/dom/rawfile \

This doesn't look necessary either. Perhaps these LOCAL_INCLUDES are needed in the other patch?

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 581680 [details] [diff] [review]
Exporting the bindings to various contexts.

Clearing review until comments are addressed.

Comment 9

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

Ok, we have decided to get rid of this binding and use js-ctypes instead.