Closed Bug 964293 Opened 6 years ago Closed 6 years ago

Reading objects from datastore has serious performance and memory consumption problems

Categories

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

x86
Gonk (Firefox OS)
defect

Tracking

()

VERIFIED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jmcf, Assigned: baku)

References

Details

(Keywords: perf, Whiteboard: [MemShrink][c=memory p= s= u=1.3])

Attachments

(4 files, 4 obsolete files)

Attached file 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.
Attached file 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.
(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.
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.
Assignee: nobody → amarchesini
Keywords: perf
Whiteboard: [MemShrink]
Attached patch cloneInto.patch (obsolete) — Splinter Review
Here a patch that makes DataStore faster. Much faster.
Attachment #8366334 - Flags: review?(bobbyholley+bmo)
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.
Flags: needinfo?(jmcf)
Hi Andrea,

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

Thanks for reacting promptly.
Flags: needinfo?(jmcf)
Attached patch cloneInto.patch (obsolete) — Splinter Review
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.
Attachment #8366334 - Attachment is obsolete: true
Attachment #8366334 - Flags: review?(bobbyholley)
Attachment #8366732 - Flags: review?(bobbyholley)
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?
Attachment #8366732 - Flags: review?(bobbyholley) → review+
Attached patch cloneInto.patch (obsolete) — Splinter Review
Ok, we have function supported. Can you take another look?
If we like this patch, we can definitely remove ObjectWrapper.jsm completely.
Attachment #8366732 - Attachment is obsolete: true
Attachment #8366827 - Flags: review?(bobbyholley)
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.
Attachment #8366827 - Flags: review?(bobbyholley) → review-
Attached patch cloneInto.patchSplinter Review
Attachment #8366827 - Attachment is obsolete: true
Attachment #8367306 - Flags: review?(bobbyholley)
cc: fxos-perf
1.3+ for Data Store and broken functionality
blocking-b2g: 1.3? → 1.3+
Priority: -- → P1
Whiteboard: [MemShrink] → [MemShrink][c= p= s= u=]
Whiteboard: [MemShrink][c= p= s= u=] → [MemShrink][c=memory p= s= u=]
Whiteboard: [MemShrink][c=memory p= s= u=] → [MemShrink][c=memory p= s= u=1.3]
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.
Attachment #8367306 - Flags: review?(bobbyholley) → review+
Blocks: 965751
https://hg.mozilla.org/mozilla-central/rev/87081cb91d97
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Attached patch aurora (obsolete) — Splinter Review
Patch for make aurora compile.
Attachment #8368317 - Flags: review?(bobbyholley)
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.
Attachment #8368317 - Flags: review?(bobbyholley) → review+
Attached patch auroraSplinter Review
Attachment #8368317 - Attachment is obsolete: true
now works much better

thanks Andrea!
Status: RESOLVED → VERIFIED
Depends on: 1016721
You need to log in before you can comment on or make changes to this bug.