1088043 - IndexedDB consistently 5 times slower than Chrome

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Peter Bengtsson [:peterbe]]

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/

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Peter Bengtsson [:peterbe]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

IndexedDB uses IPDL to communicate between threads now.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

FWIW, on release Firefox I see numbers comparable to Chrome.  Seems like PBackground fallout.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Do you know how far back this issue goes? Does it impact Firefox 34 (beta) and Firefox 31 (ESR)?

Comment 7

[Andrew McCreight [:mccr8]]

I think it is believed to be a regression from bug 994190, which landed in 35.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 :)

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Boris mentions some profile results in comment 3.

Comment 10

[Andrew Overholt [:overholt]]

While doing something for 36 would be great, I am not sure there'll be time.

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 12

[Peter Bengtsson [:peterbe]]

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

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bug 1092010 may help here.

Comment 14

[Ting-Yu Chou [:ting] (away)]

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

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: read and write nsTArrays more intelligently in IPC serialization

This patch implements something along the lines of comment 14.  Compiles, but
untested.

Comment 16

[Nathan Froyd [:froydnj]]

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.

Comment 17

[Ting-Yu Chou [:ting] (away)]

Can we use IsPod() instead?

Comment 18

[Ting-Yu Chou [:ting] (away)]

Oh, BTW, WriteParam() for the length can also be removed if you use WriteData() (and ReadData() in Read()).

Comment 19

[[:fabrice] Fabrice Desré]

Ting-Yu and Nathan, you rock!

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Also, totally worth taking that patch on 36, imo.  Though we may want the INT_MAX thing then.

Comment 22

[Jan Varga [:janv]]

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.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 24

[Nathan Froyd [:froydnj]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13d940a1c8a0

Comment 26

[Nathan Froyd [:froydnj]]

Attachment: read and write nsTArrays more intelligently in IPC serialization

Need this here for aurora approval, since it's what actually landed on inbound.

Comment 27

[Nathan Froyd [:froydnj]]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d13f3f25773