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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 2

[Jan de Mooij [:jandem]]

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?

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94f1fc3d9ec8

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-3 and 5 LSAN leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf2125ffc1c

https://treeherder.mozilla.org/logviewer.html#?job_id=7542869&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=7542871&repo=mozilla-inbound

Comment 5

[Brian Hackett [Laid off!]]

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

Comment 6

[Guilherme Lima]

This patch is breaking a few tests on AWFY

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9083621b0e2e

Comment 8

[Guilherme Lima]

This patch made Sunspider-tagcloud be 20% slower with unboxed-objects than without it.

Comment 9

[Brian Hackett [Laid off!]]

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