Closed
Bug 551982
Opened 13 years ago
Closed 12 years ago
Generate t.name and t.fields lazily
Categories
(Core :: js-ctypes, defect, P3)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: dwitte)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(4 files)
3.57 KB,
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
9.62 KB,
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
24.66 KB,
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
24.89 KB,
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → dwitte
Assignee | ||
Comment 2•12 years ago
|
||
Generates 't.name' lazily for all types.
Attachment #442539 -
Flags: review?(mozilla)
Assignee | ||
Comment 3•12 years ago
|
||
(And by all, I mean Function, Struct, Array, and Pointer types.)
Assignee | ||
Comment 4•12 years ago
|
||
Generates 't.fields' lazily for StructTypes.
Attachment #442540 -
Flags: review?(mozilla)
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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).
Attachment #442543 -
Flags: review?(mozilla)
Assignee | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 442539 [details] [diff] [review] part 1: lazy name mozilla approves
Attachment #442539 -
Flags: review?(mozilla) → review+
Updated•12 years ago
|
Attachment #442540 -
Flags: review?(mozilla) → review+
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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 11•12 years ago
|
||
comment #9 is mostly mooted by your fourth patch, so never mind that :)
Assignee | ||
Comment 12•12 years ago
|
||
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 :)
Assignee | ||
Comment 13•12 years ago
|
||
(that was in reply to comment 10, btw!)
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 442539 [details] [diff] [review] part 1: lazy name http://hg.mozilla.org/tracemonkey/rev/e051b99816ac
Assignee | ||
Comment 17•12 years ago
|
||
Comment on attachment 442540 [details] [diff] [review] part 2: lazy fields http://hg.mozilla.org/tracemonkey/rev/6bd013b63b1a
Assignee | ||
Comment 18•12 years ago
|
||
Comment on attachment 442542 [details] [diff] [review] part 3: lazy ffi_type http://hg.mozilla.org/tracemonkey/rev/6c778ba8af77
Assignee | ||
Comment 19•12 years ago
|
||
Comment on attachment 442543 [details] [diff] [review] part 4: hash StructType fields http://hg.mozilla.org/tracemonkey/rev/5ba9a76f6411
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
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.
Description
•