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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Trunk
mozilla39
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted 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.
Posted 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]
patch

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9083621b0e2e
This patch is breaking a few tests on AWFY
https://hg.mozilla.org/mozilla-central/rev/9083621b0e2e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1143106
Depends on: 1143244
Depends on: 1145426
Depends on: 1147608
Depends on: 1148916
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.