964293 - Reading objects from datastore has serious performance and memory consumption problems

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Jose Manuel Cantera]

Attachment: test1.js

Reading objects from datastore has serious time and space problems in terms of performance. The attached test demonstrates that reading from datastore an object which is an array with 1000 entries is around 11 times slower than reading it from asyncStorage (which is also supported by indexedDB). 

I'm also gonna attacha JSON file which defined an object which takes around 15 seconds to be loaded and makes the app to allocate around 30 Mbs of memory. The same object read from indexedDB takes only around 1 second to be read.

Comment 1

[Jose Manuel Cantera]

Attachment: index-comms.json

this attachment contains a big JSON file which defines an object that can be used to demonstrate the serious performance and memory problems posed by the current datastore implementation (for reading operations). To use it just read the JSON, JSON.parse and then store in a datastore. Then, try to read it. It will take around 15 seconds to read whereareas if the same operation is done through asyncStorage (supported by idb), it will be around 1.5 seconds.

Comment 2

[Jose Manuel Cantera]

(In reply to Jose M. Cantera from comment #1)
> Created attachment 8366000 [details]
> index-comms.json
> 
> this attachment contains a big JSON file which defines an object that can be
> used to demonstrate the serious performance and memory problems posed by the
> current datastore implementation (for reading operations). To use it just
> read the JSON, JSON.parse and then store in a datastore. Then, try to read
> it. It will take around 15 seconds to read whereareas if the same operation
> is done through asyncStorage (supported by idb), it will be around 1.5
> seconds.

on the other jand if the memory consumption of the process is monitored it can be seen that a peak of around 30 Mbs is allocated while reading the object.

Comment 3

[Andrea Marchesini [:baku]]

I'll investigate this bug in the next days debugging what is slow internally to DataStore.
There are possible solutions: I'm working on the rewriting of DataStore to C++, maybe this helps and it's enough.
Or another possible solution is to switch to sqlite instead indexedDB as backend.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: cloneInto.patch

Here a patch that makes DataStore faster. Much faster.

Comment 5

[Andrea Marchesini [:baku]]

Jose, with this patch DataStore allows to store any data but not Blob, File and other XPCOM/WebIDL objects.
Do you think it's a problem for apps at the moment? I can implement something but I would like to know if there are needs for it first. Thanks.

Comment 6

[Jose Manuel Cantera]

Hi Andrea,

We need to store blobs as well, for instance the Facebook photos. 

Thanks for reacting promptly.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: cloneInto.patch

This new patch supports blobs, files and filelists.

We could replace ObjectWrapper.jsm with the Cu.cloneInto if we add the support for functions. I'll do that in a follow up.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8366732 [details] [diff] [review]
cloneInto.patch

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

Looks great. r=bholley with comments addressed.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3476,5 @@
> +                             uint32_t data,
> +                             void* closure)
> +{
> +    if (tag == mozilla::dom::SCTAG_DOM_BLOB || tag == mozilla::dom::SCTAG_DOM_FILELIST) {
> +        NS_ASSERTION(!data, "Data should be empty");

MOZ_ASSERT

@@ +3480,5 @@
> +        NS_ASSERTION(!data, "Data should be empty");
> +
> +        nsISupports *supports;
> +        if (JS_ReadBytes(reader, &supports, sizeof(supports))) {
> +            JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx));

nit - no JS:: prefixing in XPConnect.

@@ +3485,5 @@
> +            if (global) {
> +                JS::Rooted<JS::Value> val(cx);
> +                if (NS_SUCCEEDED(nsContentUtils::WrapNative(cx, global, supports, &val))) {
> +                    return val.toObjectOrNull();
> +                }

Nit - no braces.

@@ +3500,5 @@
> +                              JS::Handle<JSObject*> obj,
> +                              void *closure)
> +{
> +    nsCOMPtr<nsIXPConnectWrappedNative> wrappedNative;
> +    nsContentUtils::XPConnect()-> GetWrappedNativeOfJSObject(cx, obj, getter_AddRefs(wrappedNative));

Nit - Remove the space after the ->

@@ +3522,5 @@
> +
> +    return false;
> +}
> +
> +static JSStructuredCloneCallbacks CloneIntoCallbacks = {

Add a comment above here indicating that these functions serialize raw XPCOM pointers in the data stream, and thus should only be used when the read and write are done together synchronously.

::: js/xpconnect/tests/chrome/test_cloneInto.xul
@@ +70,5 @@
> +    }
> +
> +    if (type == 'file' || type == 'blob') {
> +      is(a, b, 'File and Blob match');
> +      ok ( a === b, 'They should not match');

Seems like we can skip the first check. And the second check needs a new description ;-)

@@ +81,5 @@
> +      var aProps = [];
> +      for (var p in a) aProps.push(p);
> +
> +      var bProps = [];
> +      for (var p in b) bProps.push(p);

Might as well also sort aProps and bProps and then compare the toSource of the two arrays.

@@ +103,5 @@
> +    null,
> +    true,
> +    'hello world',
> +    [1, 2, 3],
> +    { a: 1, b: 2 },

Maybe make this object structure a bit deeper to test the recursive aspect?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: cloneInto.patch

Ok, we have function supported. Can you take another look?
If we like this patch, we can definitely remove ObjectWrapper.jsm completely.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8366827 [details] [diff] [review]
cloneInto.patch

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

::: js/xpconnect/idl/xpccomponents.idl
@@ +552,5 @@
>      [implicit_jscontext]
>      jsval getJSEngineTelemetryValue();
> +
> +    [implicit_jscontext]
> +    jsval cloneInto(in jsval value, in jsval scope, [optional] in boolean cloneFunction);

This should be an options object, so that this API doesn't keep growing boolean params.

::: js/xpconnect/src/Sandbox.cpp
@@ +1805,5 @@
>  }
>  
> +bool
> +xpc::NewFunctionForwarder(JSContext *cx, const char *name, HandleObject callable, bool doclone,
> +                          MutableHandleValue vp)

I'm not really wild about duplicating all the logic here. I think I'd rather just have the caller do:

RootedId emptyId(cx);
if (!JS_ValueToId(cx, JS_GetEmptyStringValue(cx), &emptyId))
  return false;

And then call the id overload.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3468,5 @@
>      rval.setObject(*obj);
>      return NS_OK;
>  }
>  
> +#define SCTAG_FUNCTION JS_SCTAG_USER_MIN

This should go in StructuredCloneTags.h.

@@ +3470,5 @@
>  }
>  
> +#define SCTAG_FUNCTION JS_SCTAG_USER_MIN
> +
> +class CloneIntoCallbacksData

MOZ_STACK_CLASS

@@ +3545,5 @@
> +    }
> +
> +    if (data->mCloneFunction && JS_ObjectIsCallable(cx, obj)) {
> +        RootedValue value(cx);
> +        if (!xpc::NewFunctionForwarder(cx, "[clonedFunction]", obj, false, &value))

You need to create this function in the target compartment.

::: js/xpconnect/tests/chrome/test_cloneInto.xul
@@ +132,5 @@
> +    ok(true, 'Function should not be cloned by default');
> +  }
> +
> +  var test = { a: function() { return 42; } }
> +  var output = Cu.cloneInto(test, window, true);

We need a cross-compartment version of this as well. Use Cu.Sandbox.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: cloneInto.patch

Comment 12

[Sandip Kamat]

cc: fxos-perf

Comment 13

[Preeti Raghunath(:Preeti)]

1.3+ for Data Store and broken functionality

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8367306 [details] [diff] [review]
cloneInto.patch

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

r=bholley with comments addressed. But please address them, because this patch as it stands right now introduces a sec-crit. ;-)

::: js/xpconnect/idl/xpccomponents.idl
@@ +554,5 @@
> +
> +    /*
> +     * Clone an object into a scope.
> +     * The 3rd argument is an optional options object:
> +     * - cloneFunction: boolean. If true, any function in the value is cloned.

Explain that functions are wrapped in a function forwarder that appears to be a native function in the content scope.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3526,5 @@
> +      MOZ_ASSERT(value < data->mFunctions.length());
> +
> +      RootedValue functionValue(cx);
> +      RootedObject obj(cx, data->mFunctions[value]);
> +      if (!xpc::NewFunctionForwarder(cx, obj, false, &functionValue))

This will create a cross-compartment edge! You need to wrap |obj|. Gabor made this same mistake a few months ago, so we should make sure we assert against it this time. Please do the following:

(1) In SetFunctionNativeReserved, add:
MOZ_ASSERT_IF(val.isObject(), val.toObject().compartment() == fun->compartment());
MOZ_ASSERT_IF(val.isString(), val.toString()->compartment() == fun->compartment());

(2) Verify that your tests trigger that assertion.

(3) Add a call to JS_WrapObject before calling NewFunctionForwarder.

@@ +3555,5 @@
> +        if (blob)
> +            scTag = mozilla::dom::SCTAG_DOM_BLOB;
> +
> +        nsCOMPtr<nsIDOMFileList> list = do_QueryInterface(supports);
> +        if (list)

Might as well put the QI into an |else if| to avoid doing it if we already found a blob.

@@ +3574,5 @@
> +    return false;
> +}
> +
> +// These functions serialize raw XPCOM pointers in the data stream, and thus
> +// should only be used when the read and write are done together synchronously

Nit - needs a period.

@@ +3603,5 @@
> +                                                        : nullptr);
> +    CloneIntoOptions options(aCx, optionsObject);
> +    if (!options.Parse()) {
> +        return NS_ERROR_FAILURE;
> +    }

Nit: No braces for 1-line consequents in XPConnect.

Sorry for having to nit this - thankfully, this will all be fixed soon once we run the whole tree through clang-format.

::: js/xpconnect/tests/chrome/test_cloneInto.xul
@@ +132,5 @@
> +    ok(true, 'Function should not be cloned by default');
> +  }
> +*/
> +
> +  var test = { a: function() { return 42; } }

I think we need to remove the above, and uncomment the big block above it, right? We should be running the whole spectrum of tests for both the cross-compartment and same-compartment cases.

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87081cb91d97

https://tbpl.mozilla.org/?tree=Try&rev=ac55c55902f8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87081cb91d97

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/72dc0d6848d6

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/1a658a2342be

https://tbpl.mozilla.org/php/getParsedLog.php?id=33837438&tree=Mozilla-Aurora

Comment 19

[Andrea Marchesini [:baku]]

Attachment: aurora

Patch for make aurora compile.

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8368317 [details] [diff] [review]
aurora

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

r=me with that fixed.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3702,3 @@
>              return NS_ERROR_FAILURE;
> +
> +        *aCloned = cloned;

This line should be removed.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: aurora

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7134e66035ec

Comment 23

[Jose Manuel Cantera]

now works much better

thanks Andrea!