551982 - Generate t.name and t.fields lazily

Status: RESOLVED FIXED

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[[e-mail]]

P3, nice to have.

Comment 2

[[e-mail]]

Created attachment 442539 [details] [diff] [review]
part 1: lazy name

Generates 't.name' lazily for all types.

Comment 3

[[e-mail]]

(And by all, I mean Function, Struct, Array, and Pointer types.)

Comment 4

[[e-mail]]

Created attachment 442540 [details] [diff] [review]
part 2: lazy fields

Generates 't.fields' lazily for StructTypes.

Comment 5

[[e-mail]]

Created attachment 442542 [details] [diff] [review]
part 3: lazy ffi_type

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.

Comment 6

[[e-mail]]

Created attachment 442543 [details] [diff] [review]
part 4: hash StructType fields

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;
  h.init(n);
  while (n--)
    h.add(...);

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).

Comment 7

[[e-mail]]

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.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 442539 [details] [diff] [review]
part 1: lazy name

mozilla approves

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

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
       return;
 
     Array<FieldInfo>* fields =
       static_cast<Array<FieldInfo>*>(JSVAL_TO_PRIVATE(slot));
-    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;
     }
 
     break;
   }

Just a thought.

Comment 10

[Ben Newman (:bnewman) (:benjamn)]

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.

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

comment #9 is mostly mooted by your fourth patch, so never mind that :)

Comment 12

[[e-mail]]

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 :)

Comment 13

[[e-mail]]

(that was in reply to comment 10, btw!)

Comment 14

[Ben Newman (:bnewman) (:benjamn)]

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 15

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 442543 [details] [diff] [review]
part 4: hash StructType fields

Discussed comment 14 on IRC.  Trivial fix.  Everything else looks good here.

Comment 16

[[e-mail]]

Comment on attachment 442539 [details] [diff] [review]
part 1: lazy name

http://hg.mozilla.org/tracemonkey/rev/e051b99816ac

Comment 17

[[e-mail]]

Comment on attachment 442540 [details] [diff] [review]
part 2: lazy fields

http://hg.mozilla.org/tracemonkey/rev/6bd013b63b1a

Comment 18

[[e-mail]]

Comment on attachment 442542 [details] [diff] [review]
part 3: lazy ffi_type

http://hg.mozilla.org/tracemonkey/rev/6c778ba8af77

Comment 19

[[e-mail]]

Comment on attachment 442543 [details] [diff] [review]
part 4: hash StructType fields

http://hg.mozilla.org/tracemonkey/rev/5ba9a76f6411