900236 - More rooting of StructType fields

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

As suggested by :Terrence, more rooting fixes are required for the field list.

1) Switch to RelocatableId/PtrObject
2) Use js/Vector
3) Put FieldInfo*s in the vector.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: More rooting fixes for StructType field list.

Fixes from description.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 784041 [details] [diff] [review]
More rooting fixes for StructType field list.

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

This looks much better. I'll take another quick look after you apply this feedback to make sure I didn't miss anything.

::: js/src/builtin/BinaryData.cpp
@@ +7,5 @@
>  #include "builtin/BinaryData.h"
>  
>  #include "mozilla/FloatingPoint.h"
>  
> +#include "js/Vector.h"

Sort this include into the list right after gc/Marking.h.

@@ +212,5 @@
>  Class js::NumericTypeClasses[NUMERICTYPES] = {
>      BINARYDATA_FOR_EACH_NUMERIC_TYPES(BINARYDATA_NUMERIC_CLASSES)
>  };
>  
> +typedef Vector<FieldInfo*> FieldList;

The purpose of Relocatable is to allow the pointer to be stored anywhere, even in C-managed storage like Vector and the stack. We don't use them everywhere because they are somewhat slower than Heap. The result is that you don't need to store these as pointers in the Vector. You could use them bare on the stack too, but you can avoid the Relocatable overhead, by creating a pointer instead.

e.g. Since operator[] returns a reference, you can do:
FieldInfo *info = &(*list)[i];

@@ +1512,5 @@
>          return false;
>      }
>  
>      // All error branches from here onwards should |goto error;| to free this list.
> +    FieldList *fieldList = new FieldList(cx);

Use js_new<FieldList>(cx), instead of raw |new|.

@@ +1557,5 @@
>  
>          JS_ASSERT(fieldTypeBytes.isInt32());
>          structByteSize += fieldTypeBytes.toInt32();
>  
> +        FieldInfo *info = new FieldInfo;

You shouldn't need this allocation anymore. You will need to store the FieldInfo on the stack in order to append it, but this is fine because FieldInfo only contains Relocatable now.

@@ +1619,4 @@
>          return ReportTypeError(cx, from, exemplar);
>      }
>  
> +    FieldInfo *info = NULL;

Please move this down below the for loop so that it is scoped to its usage. If it is actually supposed to be used later, then it is getting masked by the FieldInfo below and is probably a bug.

@@ +1626,5 @@
>          }
>      }
>  
> +    for (uint32_t i = 0; i < fieldList->length(); ++i) {
> +        FieldInfo *info = (*fieldList)[i];

This shadows the other |info| above, so there is probably a bug somewhere below. Either move the above |info| into a tighter scope or rename this |info|.

@@ +1632,4 @@
>  
>          RootedValue fromProp(cx);
>          if (!JSObject::getProperty(cx, valRooted, valRooted,
>                                     fieldName, &fromProp)) {

{ on new line since the if is two lines.

@@ +1640,2 @@
>          if (!ConvertAndCopyTo(cx, fieldType, fromProp,
> +                              (uint8_t *) mem + info->offset)) {

|mem| is already uint8_t*, so you shouldn't need this cast. Also { on new line, as above.

@@ +1725,5 @@
> +        return;
> +
> +    for (uint32_t i = 0; i < fieldList->length(); ++i) {
> +        FieldInfo *info = (*fieldList)[i];
> +        delete info;

You shouldn't need this block anymore.

@@ +1731,1 @@
>      delete fieldList;

js_delete(fieldList);

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: More rooting fixes for StructType field list.

Suggested changes.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 784184 [details] [diff] [review]
More rooting fixes for StructType field list.

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

r=me with nits applied.

::: js/src/builtin/BinaryData.cpp
@@ +14,5 @@
>  #include "jsutil.h"
>  
>  #include "gc/Marking.h"
> +
> +#include "js/Vector.h"

No space above or below, it's just a member of this list.

@@ +1567,5 @@
>          structByteSize += fieldTypeBytes.toInt32();
>  
> +        (&(*fieldList)[i])->name = fieldProps[i];
> +        (&(*fieldList)[i])->type = fieldType.get();
> +        (&(*fieldList)[i])->offset = fieldOffset;

There is no need for the extra indirection here. Just leave off the extra & and use . to access the field info:

(*fieldList)[i]).name = ...;

@@ +1638,4 @@
>  
>          RootedValue fromProp(cx);
>          if (!JSObject::getProperty(cx, valRooted, valRooted,
> +                                   fieldName, &fromProp))

Actually, I think this will easily fit within our 100 column limit. Just make this 1 line and remove the {}.

@@ +1647,2 @@
>          if (!ConvertAndCopyTo(cx, fieldType, fromProp,
> +                              mem + info->offset))

Ditto.

@@ +1729,5 @@
>  StructType::finalize(FreeOp *op, JSObject *obj)
>  {
>      FieldList *fieldList = static_cast<FieldList *>(obj->getPrivate());
> +    if (!fieldList)
> +        return;

It is safe to call js_delete with a NULL pointer, so remove this test.

@@ +2000,5 @@
>                               JSMSG_UNDEFINED_PROP, IdToString(cx, id));
>          return false;
>      }
>  
> +    uint8_t *loc = ((uint8_t *) obj->getPrivate()) + fieldInfo->offset;

Use a C++ style cast here.

uint8_t *loc = static_cast<uint8_t *>(obj->getPrivate()) + fieldInfo->offset;

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f66f92cb95fe

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f66f92cb95fe