More rooting of StructType fields

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 784041 [details] [diff] [review]
More rooting fixes for StructType field list.

Fixes from description.
Attachment #784041 - Flags: review?(terrence)
Assignee: general → nsm.nikhil
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);
Attachment #784041 - Flags: review?(terrence)
Blocks: 898608
Created attachment 784184 [details] [diff] [review]
More rooting fixes for StructType field list.

Suggested changes.
Attachment #784184 - Flags: review?(terrence)
Attachment #784041 - Attachment is obsolete: true
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;
Attachment #784184 - Flags: review?(terrence) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f66f92cb95fe

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/f66f92cb95fe
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.