851763 - Significant increase in minimum runtime size with large self-hosted objects

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

Testing with jsapi-tests/testGCOutOfMemory shows that the minimum size of a JSRuntime grows significantly when self-hosted JavaScript code includes large objects. The self-hosted parts of the Internationalization API contain a number of such objects, such as mapping tables for BCP 47 language subtags and the available locales for Collator, NumberFormat, and DateTimeFormat.

With the internationalization API fully enabled (the code in mozilla-central is still incomplete and disabled), the test has to be updated to call JS_NewRuntime with maxbytes = 696KB, while without it 512KB were sufficient.

Modifying the code that generates the availableLocales objects to only insert "en" and "en-GB" into the table, omitting the hundreds of other locales supported by ICU, brings the required maxbytes down to 560KB.

As far as I know, the self-hosted objects, with all their subobjects, are currently cloned into each new global. Is it possible to provide special treatment for objects whose content never changes, such that only the top-level reference needs to be copied?

Comment 1

[Boris Zbarsky [:bzbarsky]]

So is this basically adding several hundred KB to every compartment right now?  That seems like a pretty big hit for browser, given all the random subframe globals.  :(

Comment 2

[Nicholas Nethercote [inactive]]

Has this change landed?  areweslimyet.com is not showing anything notable, but it's only got numbers up to March 15.

And is the overhead per-compartment or per-runtime?  If it's the former, it is catastrophic.  If it's the latter, it's much less so, though it might still be a problem for B2G, which has multiple processes and numerous workers (each of which has its own runtime).

Comment 3

[Norbert Lindenberg]

We're currently landing the internationalization API piece by piece, but it's all still disabled, so you shouldn't see it in any measurements of nightly or TBPL builds.

I believe the clones are per compartment, but Till knows better what's really happening here.

Comment 4

[Jeff Walden [:Waldo]]

Clones are per-compartment.  This is part of the "smell" I referred to in bug 784293 comment 14, that we're going to have a whole bunch of duplicative shared data, when ideally the only non-shared bits should be approximately the actual Intl object and the functions hanging off of it, and off them recursively.

This isn't a blocker to Intl progress, even to enabling it in shells prior to enabling it in the browser.  But it is a blocker to shipping the Intl API in the browser.  And that's coming up pretty soon, most likely.  So we need to figure out something here sooner rather than later.

Comment 5

[Nicholas Nethercote [inactive]]

It would be good if someone could enable it locally and make some before vs. after measurements with about:memory.

Comment 6

[Jeff Walden [:Waldo]]

If necessary, there's always the short-term workaround of just moving this data into C++.  The pre-generated big tables could easily be generated as C++.  We could cache the available locales on JSRuntime pretty easily, too.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Could we stick them in some central compartment and access them using cross-compartment wrappers? I assume they're read-only. We could even enforce that with the wrappers.

Comment 8

[Till Schneidereit [:till]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Has this change landed?  areweslimyet.com is not showing anything notable,
> but it's only got numbers up to March 15.

None of the currently active self-hosted code is using any large object structures. In fact, I think it doesn't use any.

Also, objects are cloned lazily on first access, so even if it were active, almost none of the hundreds of compartments the browser creates would ever get these huge structures.


(In reply to Bill McCloskey (:billm) from comment #7)
> Could we stick them in some central compartment and access them using
> cross-compartment wrappers? I assume they're read-only. We could even
> enforce that with the wrappers.

Something like that would be ideal, yes. We could introduce an intrinsic that marks objects as read-only and thus shared. This way, we might even be able to do the cross-compartment accesses in a completely transparent way by installing the CCW into the target compartment instead of cloning the source object. Which would be nice.

Comment 9

[Norbert Lindenberg]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> It would be good if someone could enable it locally and make some before vs.
> after measurements with about:memory.

Builds are available:
- with Intl API enabled: https://tbpl.mozilla.org/?tree=Try&rev=cac9263b66c5
- without Intl API: https://tbpl.mozilla.org/?tree=Try&rev=e6fed780a709

Unfortunately I don't know how to interpret the ever-changing numbers provided by about:memory.

Comment 10

[Nicholas Nethercote [inactive]]

> Unfortunately I don't know how to interpret the ever-changing numbers
> provided by about:memory.

Is the problem that you don't know how to read about:memory, or that the numbers keep changing?  If it's the latter, you'll just have to get used to it, it's unfortunately unavoidable.  Often if you measure multiple times you can get a reasonable idea.

Comment 11

[Norbert Lindenberg]

I don't really know how to read about:memory, but the numbers I see seem to confirm Till's statement that there's no per-compartment penalty as long as the Intl API isn't used.

Here are the numbers I collected, five snapshots per build:

Build with Internationalization API *dis*abled (mozilla-central-macosx64/1364078844):

- compartments:      149,      156,      159,      160,      161
- explicit:          71.31 MB, 71.97 MB, 73.57 MB, 78.86 MB, 86.01 MB
- js-non-window:     32.44 MB, 24.31 MB, 25.16 MB, 29.27 MB, 28.58 MB
- heap-unclassified: 23.47 MB, 25.57 MB, 26.23 MB, 26.82 MB, 33.60 MB

Build with Internationalization API *en*abled (try-macosx64/25f6642da51f):

- compartments:      149,      156,      156,      156,      156
- explicit:          72.16 MB, 71.30 MB, 75.10 MB, 75.90 MB, 74.05 MB
- js-non-window:     33.00 MB, 23.70 MB, 24.65 MB, 24.66 MB, 24.63 MB
- heap-unclassified: 12.68 MB, 25.66 MB, 27.64 MB, 27.44 MB, 26.74 MB

The variation between the snapshots is higher than the difference between the builds. If there were a 200 KB increase per compartment, we'd be looking at a difference of some 30 MB; that difference is clearly not there.

Comment 12

[Nicholas Nethercote [inactive]]

Thanks for taking the measurements;  that's encouraging.

What are the numbers like when the Intl API *is* used?  We should see an increase there.  That'd be a good sanity check, as well as an interesting measurement in its own right.

Comment 13

[Norbert Lindenberg]

OK, I looked at the numbers while perusing the ECMA-402 conformance test, at the following points:

1) After opening a second window and loading test welcome page
2) after going to the Run page, which actually loads the test cases
3) during execution of the tests
4) after execution of the tests
5) after closing the second window

Build with Internationalization API *dis*abled:

- compartments:      167,   168-171,   232-244,    167-168,    177-169
- explicit:          85 MB, 83-107 MB, 166-200 MB, 153-161 MB, 152-108 MB

Build with Internationalization API *en*abled:

- compartments:      161,   161-167,   170-175,    161-162,    170-163
- explicit:          83 MB, 89-101 MB, 146-175 MB, 161-168 MB, 160-110 MB

The difference for step 3) is interesting. The test harness creates a new frame for every test case (there are 142 of them), and the tests are failing very quickly - maybe Firefox just can't keep up with cleaning up after them?

Comment 14

[Norbert Lindenberg]

Separate from overall memory consumption, there's the issue of increasing minimum JSRuntime size: When I reported this bug 9 days ago, 696 KB were still sufficient for testGCOutOfMemory to run with the Intl API enabled; now the minimum is up to 724 KB, likely because of the ParallelArray code that landed in the meantime. I also found out that the oranges in my try build were caused by another small JSRuntime in IndexedDB, and that the 512 KB there had already been doubled from 256 KB by Till in October (bug 784400).

I don't think it's nice to embedders to have them increase the minimum JSRuntime size all the time. They shouldn't really be affected by whether built-in code is implemented in C++ or in JavaScript; the JSRuntime size they see should reflect only their code and data structures.

Would it be possible to not charge memory usage resulting from the initialization of self-hosted code (anything allocated during initSelfHosting) against the maxbytes provided by the client?

Comment 15

[Norbert Lindenberg]

Attachment: Increase maxbytes for JSRuntime in IDBObjectStore and testGCOutOfMemory

As discussed over IRC today: just increase the maxbytes values for the JS_NewRuntime calls in IDBObjectStore and testGCOutOfMemory; don't pursue the idea mentioned in comment 14.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83b480db9bec

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83b480db9bec

Comment 18

[Till Schneidereit [:till]]

Waldo and I came up with a plan to tackle this:
https://lists.mozilla.org/pipermail/dev-tech-js-engine-internals/2013-March/000954.html

In short, I'll investigate the implementation of a CCW that allows access to self-hosted values in their original compartment from every other compartment. Objects marked as non-cloned (by an intrinsic) are wrapped with that CCW, which is then installed in the target compartment.

Depending on how the performance works out, this might work for all code that doesn't have to operate on global-specific data.

Comment 19

[Till Schneidereit [:till]]

Attachment: part 1: add support for wrapping self-hosted functions instead of cloning them.

This patch introduces the intrinsic `MakeWrappable` and implements the actual wrapping-not-cloning.

I verified that wrapped functions are correctly invoked in the self-hosting compartment and act on the data stored there, without cloning it.

I'll file a second patch actually using `MakeWrappable` once I figured out why constructing self-hosted objects in the self-hosting global doesn't really work.
Right now, e.g., `var list = new List()` creates an object without List's prototype. As the Intl code uses List heavily, this is kinda bad.

Comment 20

[Till Schneidereit [:till]]

Attachment: part 1: add support for wrapping self-hosted functions instead of cloning them. v2

Argh, after a weekend of debugging, it turns out the issue was, well, not really there. Or at least not in the way I thought.

What happened was that my debugging was severely mislead by the fact that dumping the constructor of an instance of a function with a prototype created with Object.create(null) doesn't dump anything. Combined with `instance.constructor instanceof Function` being false, this lead me straight into a very deep rabbit hole of mis-debuggin. Madness.

Also, for whatever reason (and I do think this is a real issue, but perhaps not a very important one), creating the List prototype upon first invokation of List (see Utilities.js) doesn't work in the self-hosting compartment.

So, after fixing this last issue by doing List's prototype initialization during self-hosting initialization (and adding support for the latest JS version to be able to use let and not pollute the namespace), everything works.

Specifically, I can add `MakeWrappable(CanonicalizeLanguageTag);` in Intl.js:392 without changing the number of passing tests in test262 or our test suites.

@Norbert: Maybe you could test this some more and add MakeWrappable to appropriate functions in Intl.js to reduce the amount of cloned objects?

Comment 21

[Norbert Lindenberg]

With `MakeWrappable(CanonicalizeLanguageTag);`, I'm seeing some problems:

- The jsapi-tests test testGCOutOfMemory still fails to initialize a JSRuntime with 1024 KB; without the MakeWrappable call the magic number to succeed today is 724 KB (error message: "too much recursion").

- The jsapi-tests test testOOM now also fails to initialize (same error message).

- The time to run the jstests subsuite test402 went up from about 20 to about 21 seconds on my Mac.

Comment 22

[Norbert Lindenberg]

(In reply to Till Schneidereit [:till] from comment #20)

> Also, for whatever reason (and I do think this is a real issue, but perhaps
> not a very important one), creating the List prototype upon first invokation
> of List (see Utilities.js) doesn't work in the self-hosting compartment.
> 
> So, after fixing this last issue by doing List's prototype initialization
> during self-hosting initialization (and adding support for the latest JS
> version to be able to use let and not pollute the namespace), everything
> works.

This seems to be bug 837941. I've extracted the modified List prototype initialization as a workaround for that bug.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 731655 [details] [diff] [review]
part 1: add support for wrapping self-hosted functions instead of cloning them. v2

Review of attachment 731655 [details] [diff] [review]:
-----------------------------------------------------------------

The return-meaning confusion of maybeWrappedBlah is the only real thing here I don't like -- mostly this looks sensible.

::: js/src/jsapi.cpp
@@ +5049,5 @@
>                  return JS_FALSE;
> +            RootedPropertyName shName(cx, shAtom->asPropertyName());
> +            RootedFunction fun(cx);
> +            fun = cx->runtime->maybeWrappedSelfHostedFunction(cx, shName);
> +            if (fun) {

Something's wrong here.  Isn't null return value supposed to indicate the error case?  And indeed it does, in one part of that method.  And not, in another.  I think you want this function returning bool like everything else, and maybe passing out a function through outparam.

@@ +5070,1 @@
>                                         ObjectValue(*fun), NULL, NULL, 0))

Could you use JSObject::defineProperty with shName here instead?

::: js/src/jsfun.h
@@ +100,5 @@
>      bool isSelfHostedBuiltin()      const { return flags & SELF_HOSTED; }
>      bool isSelfHostedConstructor()  const { return flags & SELF_HOSTED_CTOR; }
>      bool hasRest()                  const { return flags & HAS_REST; }
>      bool hasDefaults()              const { return flags & HAS_DEFAULTS; }
> +    bool isWrappable()              const { return flags & SH_WRAPPABLE; }

Assert that if |flags & SH_WRAPPABLE|, isSelfHostedBuiltin(), please.

::: js/src/vm/SelfHosting.cpp
@@ -216,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>      RootedValue val(cx, args[0]);
>      js_DumpValue(val);
> -    fprintf(stderr, "\n");

js_DumpValue prints a newline as well, I take it?

@@ +742,5 @@
> +    if (!GetUnclonedValue(cx, shg, id, &funVal))
> +        return NULL;
> +
> +    JS_ASSERT(funVal.isObject());
> +    JS_ASSERT(funVal.toObject().isCallable());

isFunction() instead?  That'd subsume a callable check, and be clearer to boot, I think.

@@ +744,5 @@
> +
> +    JS_ASSERT(funVal.isObject());
> +    JS_ASSERT(funVal.toObject().isCallable());
> +
> +    RootedObject funObj(cx, &funVal.toObject());

Do we have rooted functions, maybe?  Use one if so.  Can't remember if we do, offhand.

@@ +748,5 @@
> +    RootedObject funObj(cx, &funVal.toObject());
> +    if (!funObj->toFunction()->isWrappable() || !cx->compartment->wrap(cx, funObj.address()))
> +        return NULL;
> +    // For wrapped objects, `isFunction()` always returns `false`. Casting works, though.
> +    return reinterpret_cast<JSFunction *>(funObj.get());

You should be able to use static_cast<> here.  If not, add a jsfun.h #include and you should.

Comment 24

[Till Schneidereit [:till]]

Attachment: part 1: add support for wrapping self-hosted functions instead of cloning them. v3

Comment 25

[Till Schneidereit [:till]]

Comment on attachment 732220 [details] [diff] [review]
part 1: add support for wrapping self-hosted functions instead of cloning them. v3

Bleargh, interrupted a badly set up bzexport too late to really stop it, too early to at least fully let it do its thing ...

Anyway, I changed the code quite a bit so maybeWrappedBlah doesn't deal in functions and objects, anymore: the callee converted the formerly-returned JSFunction back into a value, anyway.


> > +    bool isWrappable()              const { return flags & SH_WRAPPABLE; }
> 
> Assert that if |flags & SH_WRAPPABLE|, isSelfHostedBuiltin(), please.

In the getter? No problem in that, just unusual. I could also do (SH_WRAPPABLE|SELF_HOSTED), I guess.

> 
> ::: js/src/vm/SelfHosting.cpp
> @@ -216,5 @@
> >  {
> >      CallArgs args = CallArgsFromVp(argc, vp);
> >      RootedValue val(cx, args[0]);
> >      js_DumpValue(val);
> > -    fprintf(stderr, "\n");
> 
> js_DumpValue prints a newline as well, I take it?

You take it correctly.

> 
> @@ +742,5 @@
> > +    if (!GetUnclonedValue(cx, shg, id, &funVal))
> > +        return NULL;
> > +
> > +    JS_ASSERT(funVal.isObject());
> > +    JS_ASSERT(funVal.toObject().isCallable());
> 
> isFunction() instead?  That'd subsume a callable check, and be clearer to
> boot, I think.

Probably something that should be fixed in the proxy code, but isFunction() always returns false for proxies. :(

All this is removed in the new patch, so doesn't matter much here, anymore.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 732220 [details] [diff] [review]
part 1: add support for wrapping self-hosted functions instead of cloning them. v3

Review of attachment 732220 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +5055,5 @@
> +                if (!JSObject::defineProperty(cx, obj, atom->asPropertyName(), funVal,
> +                                             NULL, NULL, flags & ~JSFUN_FLAGS_MASK))
> +                {
> +                    return JS_FALSE;
> +                }

Looking at the other half here, these will show up when decompiled as [native code], correct?  That is, just putting a wrapper here isn't going to cause a fun_toString trap or whatever to be called to expose the actual source code, or the fact that it was self-hosted.

@@ +5069,2 @@
>              RootedObject holder(cx, cx->global()->intrinsicsHolder());
> +            if (!JSObject::defineProperty(cx, holder, shAtom->asPropertyName(), funVal))

shName

::: js/src/jsfun.h
@@ +100,5 @@
>      bool isSelfHostedBuiltin()      const { return flags & SELF_HOSTED; }
>      bool isSelfHostedConstructor()  const { return flags & SELF_HOSTED_CTOR; }
>      bool hasRest()                  const { return flags & HAS_REST; }
>      bool hasDefaults()              const { return flags & HAS_DEFAULTS; }
> +    bool isWrappable()              const { return flags & SH_WRAPPABLE; }

Yeah, I meant add the assert in the getter.  It's just a double-check at an opportune time.  Always good to double-check things even past the case where it's set, for better coverage.

::: js/src/vm/SelfHosting.cpp
@@ +151,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JS_ASSERT(args.length() >= 1);
> +    JS_ASSERT(args[0].isObject());
> +    JS_ASSERT(args[0].toObject().isFunction());
> +    args[0].toObject().toFunction()->setIsWrappable();

I might name the JSFunction method makeWrappable -- setIs is a bit weird on second look.

Make sure to args.rval().setUndefined() here, as that's part of the JSNative contract (not enforced now, but we'll add the asserts at some point).

Comment 27

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52e4c977856c

> Looking at the other half here, these will show up when decompiled as
> [native code], correct?  That is, just putting a wrapper here isn't going to
> cause a fun_toString trap or whatever to be called to expose the actual
> source code, or the fact that it was self-hosted.

Correct.

> Yeah, I meant add the assert in the getter.  It's just a double-check at an
> opportune time.  Always good to double-check things even past the case where
> it's set, for better coverage.

Fair

Comment 28

[Till Schneidereit [:till]]

Norbert, can you prepare a patch that makes as many functions in Intl.js (and perhaps Utilities.js?) wrappable as possible?

Note that wrappable functions invoke all their callees in the self-hosting compartment (without forcing them to never be cloned). Also, wrapped functions will probably be slower when operating on data in client compartments, so performance-critical functions should still be cloned.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52e4c977856c

Comment 30

[Norbert Lindenberg]

With sources refreshed from m-c, I still see the jsapi-tests failures reported in comment 21.

To reproduce:
- Mac OS 10.7.5, latest Xcode & command line tools.
- m-c rev 445d8eecdd80
- bug 724533 attachment 733535 [details] [diff] [review]
- bug 837950 attachment 728870 [details] [diff] [review]
- bug 853301 attachment 729984 [details] [diff] [review]
- add `MakeWrappable(CanonicalizeLanguageTag);` after CanonicalizeLanguageTag in Intl.js
- cd js/src; autoconf213
- configure --enable-intl-api --enable-debug --disable-optimize
- cd $BUILD_DIR; make
- cd jsapi-tests; make check

Comment 31

[Andrew McCreight [:mccr8]]

Can this be closed now?  It has had a few patches landed now.

Comment 32

[Norbert Lindenberg]

(In reply to Andrew McCreight [:mccr8] from comment #31)
> Can this be closed now?  It has had a few patches landed now.

No, success is not determined by the number of patches.

(In reply to Norbert Lindenberg from comment #30)
> With sources refreshed from m-c, I still see the jsapi-tests failures
> reported in comment 21.

Same failures still occur, now without additional patches because the internationalization API is now enabled by default for standalone JS builds. Just add `MakeWrappable(CanonicalizeLanguageTag);`.

Till, any thoughts?

Comment 33

[Nicholas Nethercote [inactive]]

We're looking at this in the MemShrink meeting, and it doesn't seem like a MemShrink:P1.  The concern was that there would be a memory hit for every compartment, but measurements in comments 11 and 13 indicate not.  Is there just a hit for runtimes?  That's still not ideal, but much less of a concern.  Maybe this should be a MemShrink:P2 instead.

Comment 34

[Till Schneidereit [:till]]

The hit is just for runtimes, yes. Compartments only take a hit insofar as they use self-hosted code. Which the vast majority should do pretty sparingly. Importantly, all the i18n data is only cloned (and then only partly) when i18n functionality is used.

Having said that, bug 886193 should help with that smaller per-compartment hit.

Comment 35

[Nicholas Nethercote [inactive]]

> The hit is just for runtimes, yes.

Thanks for the info.  I'll downgrade to MemShrink:P2 accordingly.

Comment 36

[Till Schneidereit [:till]]

Waldo, I think we should close this bug. We're not going to use MakeWrappable, and the memory usage hit here is only going to be taken once per process once bug 964057 lands. Do you agree?

Comment 37

[Jeff Walden [:Waldo]]

Sure, sounds fine.