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

(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)?

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.

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

Bug 1092010 may help here.

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

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

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.

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.

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.

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.

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

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.