Closed Bug 551982 Opened 13 years ago Closed 12 years ago

Generate and t.fields lazily


(Core :: js-ctypes, defect, P3)

Other Branch





(Reporter: jorendorff, Assigned: dwitte)



(Whiteboard: fixed-in-tracemonkey)


(4 files)

Bug 513788, comments 38 and 42:
>> Can we compute the name on first use instead? Same question in
>> ArrayType::CreateInternal, and in StructType::Create regarding
>> t.fields. These introspection features will probably be used mainly during
>> development; it would be nice to optimize the cost away once the add-on is
>> deployed. If you agree, I can file a follow-up bug.
> Sure, followup. I tried doing that initially, but it led to some complications.
> I forget exactly what.
P3, nice to have.
Priority: -- → P3
Assignee: nobody → dwitte
Generates '' lazily for all types.
Attachment #442539 - Flags: review?(mozilla)
(And by all, I mean Function, Struct, Array, and Pointer types.)
Generates 't.fields' lazily for StructTypes.
Attachment #442540 - Flags: review?(mozilla)
Bonus patch: generate the ffi_type lazily (all except for those associated with a FunctionType). A side effect of this is that instantiating an array or struct type no longer requires O(n) memory, thusly the tests can be a little more direct.
Attachment #442542 - Flags: review?(mozilla)
Continuing the abuse of the scope of this bug, another bonus patch: switch out the array of FieldInfos for a hash. This gets rid of potential O(n^2) behavior, and given the lightweightedness of js::HashMap, should still be pretty fast for the few-fields case.

There is one caveat with this patch:

  HashMap<...> h;
  while (n--)

If 'init' succeeded, the 'add' calls may not necessarily do so, since the hash may resize according to alpha. 'init' does not take this into account when determining the number of buckets, but it should. I'll file a followup on this, but for now in this patch, I assume that it works (and thus merely assert success).
Attachment #442543 - Flags: review?(mozilla)
I was thinking about making 't.prototype' also generated lazily. But I think the win from this will be minimal. When one declares a type, one probably wishes to access data from it -- either via data coming from C, or by instantiating it directly. For structs, if one doesn't (e.g. passing a pointer to an opaque struct, array, etc), one should just declare an opaque type, which will not result in a prototype being created.

Assuming that is true, then for all types, the win will be eliminating one JS_NewObject and one JS_DefineProperty call. Not sure if that's worth it.
Depends on: 562809
Comment on attachment 442539 [details] [diff] [review]
part 1: lazy name

mozilla approves
Attachment #442539 - Flags: review?(mozilla) → review+
Attachment #442540 - Flags: review?(mozilla) → review+
Comment on attachment 442540 [details] [diff] [review]
part 2: lazy fields

Also looks good to me.

One of your idioms that occurs outside this patch (but that this patch brought to my attention) is iteration over Array<FieldInfo>s using a size_t index and the length() and begin() methods.

Sometimes you need that index, but mostly you don't.  You can get rid of it if you switch to the following idiom:

diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp
--- a/js/src/ctypes/CTypes.cpp
+++ b/js/src/ctypes/CTypes.cpp
@@ -2669,11 +2669,12 @@ CType::Trace(JSTracer* trc, JSObject* ob
     Array<FieldInfo>* fields =
-    for (size_t i = 0; i < fields->length(); ++i) {
-      FieldInfo* field = fields->begin() + i;
+    FieldInfo* field = fields->begin(), *end = field + fields->length();
+    while (field != end) {
       JS_CALL_TRACER(trc, field->mType, JSTRACE_OBJECT, "field");
+      ++field;

Just a thought.
Comment on attachment 442542 [details] [diff] [review]
part 3: lazy ffi_type

Looks good.  Assuming we go this way, we should just INVALIDate bug 561921.
Attachment #442542 - Flags: review?(mozilla) → review+
comment #9 is mostly mooted by your fourth patch, so never mind that :)
Probably, yeah. We could save two reserved slots on CTypes if we generated ffi_types eagerly. But the cost is two (at best, one) malloc, and a little bit of work filling it out. Assuming we fixed the libffi array suckage first.

So we should probably just go with the extra two slots :)
(that was in reply to comment 10, btw!)
Comment on attachment 442543 [details] [diff] [review]
part 4: hash StructType fields

>-  // Create an array of FieldInfo objects to stash on the type object.
>-  AutoPtr< Array<FieldInfo> > fields(new Array<FieldInfo>);
>-  if (!fields || !fields->resize(len)) {
>+  // Create a hash of FieldInfo objects to stash on the type object.
>+  FieldInfoHash* fields(new FieldInfoHash);
>+  if (!fields || !fields->init(len)) {
>+    delete fields;
>     JS_ReportOutOfMemory(cx);
>     return JS_FALSE;
>   }
>+  // Stash the FieldInfo hash in a reserved slot now, for GC safety of its
>+  // constituents.
>+  if (!JS_SetReservedSlot(cx, typeObj, SLOT_FIELDINFO,
>+         PRIVATE_TO_JSVAL(fields)))
>+    return JS_FALSE;

Leak risk?  The idiom elsewhere would recommend switching back to |AutoPtr<FieldInfoHash> fields(new FieldInfoHash);|.  The only concern is that you might free |fields| due to a failure later on, leaving a dangling pointer in SLOT_FIELDINFO.  So just set the reserved slot after everything else has succeeded?

r=me with that
Comment on attachment 442543 [details] [diff] [review]
part 4: hash StructType fields

Discussed comment 14 on IRC.  Trivial fix.  Everything else looks good here.
Attachment #442543 - Flags: review?(mozilla) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
You need to log in before you can comment on or make changes to this bug.