Closed Bug 1135897 Opened 7 years ago Closed 7 years ago

Use unboxed objects for JSON objects and constant literals embedded in scripts


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)




(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
In a given compartment, it should be common for JSON objects and constant-membered objects in scripts to have the same property types if they have the same property names.  We should try to use unboxed objects for these, which will improve speed and memory usage when using these objects.  The attached patch makes this change.

It does, however, slow down JSON parsing a bit.  kraken-json-parse-financial is about 5% slower for me with this patch.  While this isn't that big an issue, JSON parsing is one of those things where we should strive to be as fast as possible.  I think it would be neat if, once we decide to use an unboxed representation for objects with some list of properties and made some large number of them (1000, say), we compiled a custom jitcode stub that could create the object and fill it in from the vector of values which is constructed during the JSON parse.  I'll write another patch on top of this one to try out this idea.
Attached patch patchSplinter Review
Updated patch which makes JIT stubs for constructing unboxed objects while parsing JSON or script sources.  If I run kraken-json-parse-financial ten times as long on x86 10.9, I get 470ms before this patch (both with/without unboxed objects), 460ms after this patch without unboxed objects, and 410ms after this patch with unboxed objects.

We could improve this in the future to e.g. allow these JIT stubs to create boxed objects as well, but the heuristics involved and management of the JitCode* make this kind of complicated so this patch does the simple thing.
Assignee: nobody → bhackett1024
Attachment #8568209 - Attachment is obsolete: true
Attachment #8571430 - Flags: review?(jdemooij)
Comment on attachment 8571430 [details] [diff] [review]

Review of attachment 8571430 [details] [diff] [review]:

::: js/src/jit/MacroAssembler.cpp
@@ +1266,5 @@
>              offset += sizeof(uintptr_t);
>          }
>      } else if (templateObj->is<UnboxedPlainObject>()) {
> +        if (initFixedSlots)
> +            initUnboxedObject(obj, &templateObj->as<UnboxedPlainObject>());

Hm can initFixedSlots really be false here? If we *are* using it for unboxed objects as well, we should maybe rename it...

::: js/src/vm/UnboxedObject.cpp
@@ +540,5 @@
>      return res;
>  }
> +/* static */ JSObject *
> +UnboxedPlainObject::createWithProperties(ExclusiveContext *cx, HandleObjectGroup group,

Can you make sure this patch compiles/works with --disable-ion?
Attachment #8571430 - Flags: review?(jdemooij) → review+
Hmm, the malloc'ed data belonging to the plain object type table wasn't being freed for compartments used by off thread parses.  I don't know why we weren't hitting this before.
This patch is breaking a few tests on AWFY
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1143244
This patch made Sunspider-tagcloud be 20% slower with unboxed-objects than without it.
(In reply to Guilherme Lima from comment #8)
> This patch made Sunspider-tagcloud be 20% slower with unboxed-objects than
> without it.

The patch in bug 1157703 should fix this regression.
You need to log in before you can comment on or make changes to this bug.