Bug 97738

Created attachment 165893 [details] Patch

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

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.

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

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

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

Created attachment 166013 [details] Patch

Comment on attachment 166013 [details] Patch LGTM

tony@ - r?

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.

Created attachment 166093 [details] Patch for landing

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

Created attachment 166265 [details] Patch for landing

Comment on attachment 166265 [details] Patch for landing Clearing flags on attachment: 166265 Committed r129911: <http://trac.webkit.org/changeset/129911>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 97933

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

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

Created attachment 174015 [details] Patch for landing

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.

Comment on attachment 174015 [details] Patch for landing Clearing flags on attachment: 174015 Committed r134529: <http://trac.webkit.org/changeset/134529>

All reviewed patches have been landed. Closing bug.

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