IndexedDB consistently 5 times slower than Chrome

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: peterbe, Assigned: froydnj)

Tracking

({perf, regression})

unspecified
mozilla36
x86
macOS
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox33 unaffected, firefox34 unaffected, firefox35+ fixed, firefox36+ fixed, firefox-esr31 unaffected, b2g-v2.1 unaffected, b2g-v2.2 fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
This demo page, which was meant to compare the difference between IndexedDB vs. AJAX, shows pretty consistent numbers in Firefox 35 (Aurora) and Chrome 38. 

http://www.peterbe.com/localforage-vs-xhr/index.html

In Fx35 I get on average 28ms to retrieve but only 7ms in Chrome. 

Frankly I'm using a wrapper on IndexedDB called localForage [0] so there might be other places where the 28ms are spent that isn't about IndexedDB. 

[0] https://github.com/mozilla/localforage/
Peter, could you post a testcase that skips the AJAX bits and just does the part we're trying to measure here?  That would make profiling a bit simpler....
(Reporter)

Comment 2

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #1)
> Peter, could you post a testcase that skips the AJAX bits and just does the
> part we're trying to measure here?  That would make profiling a bit
> simpler....

Like this?
http://www.peterbe.com/just-localforage/index.html

I guess I could try to understand how localforage implements its getItem and do that "in the raw" if you think that would help.
That helps, yes.  There's still a bunch of noise in the profile because of all the other work we do between the gets, but...

Anyway, something that looks a bit weird to me: we're spending a noticeable amount of time under indexedDB::PBackgroundIDBRequestParent::Send__delete__.  This is doing a PBackgroundIDBRequestParent::Write() which ends up in Pickle::WriteBytes, which is doing various memmove, realloc, bzero, etc bits.  Interestingly, I never see WriteBytes calls for more than a few bytes at a time.
IndexedDB uses IPDL to communicate between threads now.
FWIW, on release Firefox I see numbers comparable to Chrome.  Seems like PBackground fallout.
Do you know how far back this issue goes? Does it impact Firefox 34 (beta) and Firefox 31 (ESR)?
Flags: needinfo?(peterbe)
I think it is believed to be a regression from bug 994190, which landed in 35.
I don't think we're going to be able to do anything here for 35, we're kinda swamped. Maybe for 36. Of course, there are no profiles here so I don't really know what the problem is :)
Boris mentions some profile results in comment 3.
While doing something for 36 would be great, I am not sure there'll be time.
Based on comment 8, I'm marking 35 as wontfix. This will most likely be addressed in 37. I'm leaving tracked for 36 until we add the tracking flag for 37.
(Reporter)

Comment 12

5 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #6)
> Do you know how far back this issue goes? Does it impact Firefox 34 (beta)
> and Firefox 31 (ESR)?

Honestly don't know. I actually don't even have Release installed.
Flags: needinfo?(peterbe)
Writing and reading the field |data| of struct SerializedStructuredCloneReadInfo takes time, it is a uint8_t nsTArray and falls to ParamTraits<FallibleTArray<E>>, which the Write() and Read() is a for loop of WriteParam() and ReadParam() for each index.

It can be improved by a WriteBytes()/ReadBytes() call when E is builtin data type (maybe POD as well).
(Assignee)

Comment 15

5 years ago
This patch implements something along the lines of comment 14.  Compiles, but
untested.
(Assignee)

Comment 16

5 years ago
Comment on attachment 8516679 [details] [diff] [review]
read and write nsTArrays more intelligently in IPC serialization

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

|mach mochitest dom/indexedDB/| passes.  The testcase from comment 2 in Nightly gives numbers:

IndexedDB 	114.72ms	180.57ms	67.48ms	102.88ms	67.93ms

Same testcase in self-compiled binary with patch gives:

IndexedDB 	18.31ms	17.60ms	16.92ms	17.22ms	26.03ms

Gonna call a 5x+ speedup a win.
Attachment #8516679 - Flags: review?(Jan.Varga)
Can we use IsPod() instead?
Oh, BTW, WriteParam() for the length can also be removed if you use WriteData() (and ReadData() in Read()).
Ting-Yu and Nathan, you rock!
Comment on attachment 8516679 [details] [diff] [review]
read and write nsTArrays more intelligently in IPC serialization

> Can we use IsPod() instead?

The real test here would be whether E has a ParamTraits that does something more than WriteBytes.  Nothing prevents POD stuff from having nontrivial ParamTraits (e.g. you can have a POD thing with a self-pointer).

The overflow problem is real, though.  Pickle::WriteBytes takes an "int" for the number of bytes, not a size_t (wtf?).

Note that this will also totally fail for WriteString if the string doesn't fit in an int and so forth...  We should probably file a followup to covert this stuff to using size_t or something.  We _could_ try to work around it here by chunking up into INT_MAX-sized chunks, but it would be better to solve the general problem if we can.
Also, totally worth taking that patch on 36, imo.  Though we may want the INT_MAX thing then.
Comment on attachment 8516679 [details] [diff] [review]
read and write nsTArrays more intelligently in IPC serialization

I'm an IndexedDB module peer, not an IPC one.
Attachment #8516679 - Flags: review?(Jan.Varga) → review?(bent.mozilla)
Comment on attachment 8516679 [details] [diff] [review]
read and write nsTArrays more intelligently in IPC serialization

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

r=me with these changes:

::: ipc/glue/IPCMessageUtils.h
@@ +470,5 @@
>      uint32_t length = aParam.Length();
>      WriteParam(aMsg, length);
> +    if (mozilla::IsIntegral<E>::value ||
> +        mozilla::IsFloatingPoint<E>::value) {
> +      // XXX what about multiplication overflow here

Can you at least MOZ_ASSERT that here?

@@ +488,5 @@
>      }
>  
> +    if (mozilla::IsIntegral<E>::value ||
> +        mozilla::IsFloatingPoint<E>::value) {
> +      E* elements = aResult->AppendElements(length);

Let's do this allocation after we call ReadBytes below, it will save us an allocation if the message is badly formed somehow.

@@ +494,5 @@
>          return false;
>        }
> +
> +      const char* outdata;
> +      // XXX what about multiplication overflow here

I think this needs to be more careful and actually return false if this were to happen. A hacked child process could send such values, and our fuzzer should hit this too.

@@ +500,5 @@
> +        return false;
> +      }
> +      memcpy(elements, outdata, length * sizeof(E));
> +    } else {
> +      aResult->SetCapacity(length);

Really weird that the old code didn't check this for failure... If you check here then you don't have to check that AppendElement is non-null in the loop.
Attachment #8516679 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 24

5 years ago
Fixed up the overflow checks and addressed other review comments:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13d940a1c8a0

The overflow checking stuff should probably move into a Pickle method so we can reuse it for ns{,C}String, among other things.
Assignee: nobody → nfroyd
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/13d940a1c8a0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Comment 26

5 years ago
Need this here for aurora approval, since it's what actually landed on inbound.
Attachment #8516679 - Attachment is obsolete: true
Attachment #8517630 - Flags: review+
(Assignee)

Comment 27

5 years ago
Comment on attachment 8517630 [details] [diff] [review]
read and write nsTArrays more intelligently in IPC serialization

Approval Request Comment
[Feature/regressing bug #]: Bug 994190, we think.
[User impact if declined]: Webpages/apps that use IndexedDB will be slow.
[Describe test coverage new/current, TBPL]: Green on inbound, m-c.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8517630 - Flags: approval-mozilla-aurora?
Attachment #8517630 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.