Bug 97738 – IndexedDB: Run multiple tasks per transaction tick

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IndexedDB: Pair of performance fixes

[Joshua Bell]

Created attachment 165893 [details]
Patch

[Joshua Bell]

alecflett@, dgrogan@ - can you take a look?

[David Grogan]

Comment on attachment 165893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165893&action=review

> Source/WebCore/ChangeLog:9
> +        via profiling in string encoding (pro-tip: if it's already UChar, just memcpy);

The old way doesn't provide robustness in the face of different endianness?  Sorry if that's a stupid question.

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:293
> +    size_t len = s.length() * sizeof(UChar);

Is there any advantage to passing len to the Vector constructor like line 292-old does?

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:258
> +    TaskQueue* taskQueue = m_pendingPreemptiveEvents ? &m_preemptiveTaskQueue : &m_taskQueue;

This seems like the type of innocuous-looking change that causes some other non-robust portion of our code to start behaving non-deterministically, but I don't really know what's going on. I'll let Alec comment.

[Joshua Bell]

(In reply to comment #3)
> (From update of attachment 165893 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165893&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        via profiling in string encoding (pro-tip: if it's already UChar, just memcpy);
> 
> The old way doesn't provide robustness in the face of different endianness?  Sorry if that's a stupid question.

It's a great question, actually.

The old method actually defends against different endianness by always reading/writing as big endian... which would be a breaking format change since most architectures are little endian. Derp.

(I was under the impression that leveldb stores weren't portable across different-endian architectures which is why I blithely made the change.)

I'll split this patch up and maybe turf this one.

> > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:293
> > +    size_t len = s.length() * sizeof(UChar);
> 
> Is there any advantage to passing len to the Vector constructor like line 292-old does?

Hrm, there probably is, since IIRC Vector has a default size.
 
> > Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:258
> > +    TaskQueue* taskQueue = m_pendingPreemptiveEvents ? &m_preemptiveTaskQueue : &m_taskQueue;
> 
> This seems like the type of innocuous-looking change that causes some other non-robust portion of our code to start behaving non-deterministically, but I don't really know what's going on. I'll let Alec comment.

[WebKit Review Bot]

Comment on attachment 165893 [details]
Patch

Attachment 165893 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14018723

New failing tests:
IDBLevelDBCodingTest.CompareEncodedStringsWithLength
storage/indexeddb/cursor-key-order.html
IDBLevelDBCodingTest.DecodeString
IDBLevelDBCodingTest.DecodeIDBKeyPath

[Joshua Bell]

(In reply to comment #5)
> (From update of attachment 165893 [details])
> Attachment 165893 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14018723
> 
> New failing tests:
> IDBLevelDBCodingTest.CompareEncodedStringsWithLength
> storage/indexeddb/cursor-key-order.html
> IDBLevelDBCodingTest.DecodeString
> IDBLevelDBCodingTest.DecodeIDBKeyPath

Awesome - we do have endianness tests, which saved the day.

I'll split this up and add a string endianness switch as a future perf item.

[Joshua Bell]

Created attachment 166013 [details]
Patch

[Alec Flett]

Comment on attachment 166013 [details]
Patch

LGTM

[Joshua Bell]

tony@ - r?

[Tony Chang]

Comment on attachment 166013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166013&action=review

> Source/WebCore/ChangeLog:9
> +        Process multiple tasks from the pending queue(s) when the timer fires. The
> +        task may initiate new tasks that change which queue is active (e.g. indexing

Per times (like the time to run all the LayoutTests before/after this change) would be nice.

[Joshua Bell]

Created attachment 166093 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 166093 [details]
Patch for landing

Rejecting attachment 166093 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9
Updating OpenSource
From http://git.chromium.org/external/Webkit
   82bc0a1..e7aca46  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 129817 = c176af8a903a16f338b44f42a58940a7dedc9084
last_rev is higher!: 129817 >= 129802 at /usr/lib/git-core/git-svn line 1523

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14034982

[Joshua Bell]

Created attachment 166265 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 166265 [details]
Patch for landing

Clearing flags on attachment: 166265

Committed r129911: <http://trac.webkit.org/changeset/129911>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 97933

[Joshua Bell]

(In reply to comment #16)
> Re-opened since this is blocked by bug 97933

Two theories so far, with no evidence:

* This change means that tasks may execute even before the previous task calls didCompleteTaskEvents if there's async operation in between (e.g. firing onSuccess across an IPC boundary). Possibly something is sensitive to that, as the InspectorIndexedDBAgent does explicitly call didCompleteTaskEvents().

* m_taskEventTimer.startOneShot(0) may be called repeatedly now without waiting for the previous call to fire. Perhaps there's something on Windows that's sensitive to this?

[Joshua Bell]

Crashy test link is http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Finspector%2Findexeddb%2Fdatabase-data.html - win debug only

[Joshua Bell]

Created attachment 174015 [details]
Patch for landing

[Joshua Bell]

Trying this again. I was never able to repro the inspector crash (locally or with the try bots). 

The IndexedDB inspector code has since been completely re-written; it used to use  back-end APIs, and needed to explicitly poke a "private" API (didCompleteTaskEvents) to signal that tasks were complete, and some re-entrancy problem induced by this platform-specific timer behavior was my suspicion for the cause of the crash. The way the inspector talks to IDB has completely changed to only use front-end APIs, so I believe that this should be safe now.

...

Gardeners - if tests start crashing after this lands, please roll it out.

[WebKit Review Bot]

Comment on attachment 174015 [details]
Patch for landing

Clearing flags on attachment: 174015

Committed r134529: <http://trac.webkit.org/changeset/134529>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Wow, this was a huge improvement:

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/indexeddb/report.html?rev=167890&graph=basic_insert&trace=t&history=150

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/indexeddb/report.html?rev=167890&graph=basic_insert&trace=t&history=150

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/indexeddb/report.html?rev=167890&graph=basic_insert&trace=t&history=150

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/indexeddb/report.html?rev=167890&graph=basic_insert&trace=t&history=150