Closed
Bug 699446
Opened 13 years ago
Closed 13 years ago
Add cache for constructing new objects from the VM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(2 files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Constructing objects in the VM is more expensive with the JM smaller object format, as certain things which used to be easily accessible but wasteful on memory (JSObject::newType, sharedNonNativeMap, ...) have their information encoded via hashtables with a relatively expensive lookup.
The attached patch adds a cache to the VM to make repetitive creation of similar objects much faster. The cache is modeled on the property cache --- a fixed number of entries where fills will clobber existing entries, and wiped on GC.
On this test (modeled after a Dromaeo test):
function foo() {
var ret = [], i = 1024;
function test() {
for ( var j = 0; j < i * 10; j++ )
ret = new Array(i);
}
for (var i = 0; i < 1000; i++)
test();
}
foo();
js -m -n (trunk): 617
js -m -n (JM old): 924
js -m -n (JM new): 562
Attachment #571679 -
Flags: review?(luke)
Assignee | ||
Updated•13 years ago
|
Blocks: ObjectShrink
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
Followup fix to inhibit this cache when constructing objects whose class does not have a cached proto key. Creating these objects requires property lookups to get global[className].prototype, where dynamic changes on either property would render the cached entry incorrect.
Assignee: general → bhackett1024
Attachment #574647 -
Flags: review?(luke)
Assignee | ||
Comment 3•13 years ago
|
||
Updated•13 years ago
|
Attachment #574647 -
Flags: review?(luke) → review+
Comment 4•13 years ago
|
||
Comment on attachment 571679 [details] [diff] [review]
patch
Review of attachment 571679 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsgcinlines.h
@@ +400,5 @@
>
> +/* Alternate form which allocates a GC thing if doing so cannot trigger a GC. */
> +template <typename T>
> +inline T *
> +TryNewGCThing(JSContext *cx, js::gc::AllocKind kind, size_t thingSize)
Could you move this (and NewGCThing above it) into namespace js?
::: js/src/jsobj.h
@@ +1509,5 @@
> + * Cache for speeding up repetitive creation of objects in the VM.
> + * When an object is created which matches the criteria in the 'key' section
> + * below, an entry is filled with the resulting object.
> + */
> +struct NewObjectCache
Could you make this be a class with private data members?
@@ +1527,5 @@
> + * - Prototype for the object (cannot be global). The object's parent
> + * will be the prototype's parent.
> + *
> + * - Type for the object. The object's parent will be the type's
> + * prototype's parent.
This is a good comment. Could you make a fill() overload for each case (they could all call a private fill() that actually did the work)? That would give static checking between cases 1,2 and case 3 and then you could have a dynamic check for global vs. non-global.
@@ +1550,5 @@
> + Entry entries[41];
> +
> + void reset() { PodZero(this); }
> +
> + bool lookup(Class *clasp, gc::Cell *key, gc::AllocKind kind, Entry **pentry)
So it's easier to read, could you define this next to NewObjectCache::Entry::fill?
@@ +1553,5 @@
> +
> + bool lookup(Class *clasp, gc::Cell *key, gc::AllocKind kind, Entry **pentry)
> + {
> + jsuword hash = (jsuword(clasp) ^ jsuword(key)) + kind;
> + Entry *entry = *pentry = &entries[hash % JS_ARRAY_LENGTH(entries)];
entries's length is not a power of 2 so this will actually do the div, not mask. Any reason not to make give entries 32 or 64 elements?
Attachment #571679 -
Flags: review?(luke) → review+
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•