Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349863003:
[sql] Use memory-mapped I/O for sql::Connection. (Closed)
|
Created:
5 years, 3 months ago by Scott Hess - ex-Googler Modified:
5 years, 3 months ago CC:
chromium-reviews, michaeln Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sql] Use memory-mapped I/O for sql::Connection.
sql::Connection::Open*() uses PRAGMA mmap_size to enable SQLite's
memory-mapped I/O. Additionally instrument to flush dirty pages from
the page cache after writes.
BUG=489784, 533682
Committed: https://crrev.com/9a1948a4d6d445d5c8e209bdcd1cd050af72060b
Cr-Commit-Position: refs/heads/master@{#350362}
Patch Set 1 #
Total comments: 1
Patch Set 2 : style nit, rebase #
Total comments: 15
Patch Set 3 : Wordsmithing, mmap_size smaller, no mmap on iOS, mmap test vs flush, fix sync test failure. #
Total comments: 3
Patch Set 4 : rebase #Patch Set 5 : name changes, disable mmap for sync. #Patch Set 6 : overreverted sync fix, needed to keep iOS fix #
Total comments: 2
Patch Set 7 : Rebase #Patch Set 8 : Speling chaneg. #
Messages
Total messages: 62 (20 generated)
shess@chromium.org changed reviewers: + rmcilroy@chromium.org
I've been procrastinating on this, but I finally sat down and groveled through my myriad local dev branches and pulled this together. I don't really care for the layer that the cache clearing is operating at. There are options, but all of the options seemed even worse (sometimes much worse). I think the best option may be to just get an implementation upstreamed under a PRAGMA mmap_something flag. I have been near-catatonic on performance. AFAICT, the biggest difference is that setting up the mmap shifts some costs forward from where they would normally be. I'm still working out the impacts of this WRT files which are changing size (requires a re-mmap), but it seems like maybe best to just jump and test it before I distract myself further. Perf tests on Linux seem improved. I'll hit up OSX and Windows now that this is up. I'm most worried about OSX, I noticed some unexpected things when micro-benchmarking. https://codereview.chromium.org/1349863003/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/1349863003/diff/1/sql/connection.cc#newcode490 sql/connection.cc:490: // around. Apologies for the mega-comment. I'm not entirely clear where else to put it.
The CQ bit was checked by shess@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shess@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/09/16 23:48:19, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Failing because a CorruptDatabase() helper is truncating the database while SQLite is still using it. Which is an interesting problem I'm not sure it worth worrying about.
Generally lgtm assuming the tests can be fixed and we are sure the f.Write operations are always reflected in the mmap immediately. https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:511: void Connection::ReleaseCacheMemoryIfNeeded(bool assume_changed) { nit - /s/assume_changed/force_release/ ? https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:1314: ignore_result(Execute("PRAGMA mmap_size = 2147483648")); // 2GB. Could we end up in a situation where this causes address space to be exhausted on a 32-bit platform if there was a massive database mapped? Just noticed that you cap SQLITE_MAX_MMAP_SIZE to 256 MB. Rather than setting this to an arbitrary 2GB here, would it be possible to share a #define both here and in third_party/sql so that both are set to the same value (not sure if this is possible given how the gyp files are setup)? https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h File sql/connection.h (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h#newcod... sql/connection.h:265: // NOTE(shess): See also ReleaseCacheMemory(). Not sure this comment is needed given ReleaseCacheMemory is private. Are there any situations where it would be worth having TrimMemory call ReleaseCacheMemory()? https://codereview.chromium.org/1349863003/diff/20001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection_unittest... sql/connection_unittest.cc:1349: ASSERT_EQ(0, memcmp(buf, m.data() + 0*sizeof(buf), sizeof(buf))); Are we sure that this atomic on all platforms we care about? it seems to me that there might be a race where if the f.Write(...) gets buffered somewhere (and not reflected in the mmap), but by the time the memcmp reads it the write is reflected in the mmap. I'm not sure how to test for this though - maybe one more test which only updates a single byte and checks that byte so there would be less time between the initial write and the initial read of the mmap?
Wordsmithing, mmap_size smaller, no mmap on iOS, mmap test vs flush, fix sync test failure.
The CQ bit was checked by shess@chromium.org to run a CQ dry run
OK, made some changes based on comments. https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:511: void Connection::ReleaseCacheMemoryIfNeeded(bool assume_changed) { On 2015/09/17 17:17:44, rmcilroy wrote: > nit - /s/assume_changed/force_release/ ? I can see being against |assume_changed|. |force_release| isn't really it, since it may not release if mmap not enabled, or if in a transaction (in that case it will release later, though). Maybe |hint_changed|? https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:1314: ignore_result(Execute("PRAGMA mmap_size = 2147483648")); // 2GB. On 2015/09/17 17:17:44, rmcilroy wrote: > Could we end up in a situation where this causes address space to be exhausted > on a 32-bit platform if there was a massive database mapped? > > Just noticed that you cap SQLITE_MAX_MMAP_SIZE to 256 MB. Rather than setting > this to an arbitrary 2GB here, would it be possible to share a #define both here > and in third_party/sql so that both are set to the same value (not sure if this > is possible given how the gyp files are setup)? I'll set both to 256MB for now. My original intention was that on 32-bit platforms SQLITE_MAX_MMAP_SIZE might be set conservatively to prevent fragmentation, but on 64-bit platforms it might be more liberal, whereas this code would set it more of an on/off setting without regard to such policy decisions. But we control horizontal and vertical, changing it is just a CL regardless of whether it touches both files or not, meh. [USE_SYSTEM_SQLITE could also change this, but I can't say how to accommodate that, beyond the following code double-checking that the result was non-zero.] https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h File sql/connection.h (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h#newcod... sql/connection.h:265: // NOTE(shess): See also ReleaseCacheMemory(). On 2015/09/17 17:17:44, rmcilroy wrote: > Not sure this comment is needed given ReleaseCacheMemory is private. Are there > any situations where it would be worth having TrimMemory call > ReleaseCacheMemory()? I believe sqlite3_db_release_memory() should match TrimMemory() when run in aggressive mode, so if the mmap stuff works as intended, then for those platforms TrimMemory() is no longer useful. I'll back out those comments, though. Mostly I'm thinking about whether TrimMemory() should be removed entirely, or implementation as always-aggressive using sqlite3_db_release_memory(), or if even the non-mmap case should be converted to always releasing the cache and relying on the filesystem buffers to make things fast (mod syscall overhead). None of that thinking-out-loud needs to be in this CL though! https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h#newcod... sql/connection.h:695: int total_changes_; I changed this to |total_changes_last_release_| to discourage anyone from assuming this is a generally-useful cache of sqlite3_total_changes(). https://codereview.chromium.org/1349863003/diff/20001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection_unittest... sql/connection_unittest.cc:1349: ASSERT_EQ(0, memcmp(buf, m.data() + 0*sizeof(buf), sizeof(buf))); On 2015/09/17 17:17:44, rmcilroy wrote: > Are we sure that this atomic on all platforms we care about? it seems to me that > there might be a race where if the f.Write(...) gets buffered somewhere (and not > reflected in the mmap), but by the time the memcmp reads it the write is > reflected in the mmap. I'm not sure how to test for this though - maybe one more > test which only updates a single byte and checks that byte so there would be > less time between the initial write and the initial read of the mmap? Ooooh, thanks for pointing this out, I hadn't thought of that issue. What I'm trying to test here is the relationship between the data passed to a system call like pwrite(), and the memory map. AFAICT both POSIX and Windows implement base::File such that this tests the correct things as-is, but it would be possible to have a base::File implementation over FILE or something, which wouldn't be a correct test. Unfortunately, the current implementation of base::File::Flush() is more like fsync() than fflush(), which is _not_ what I want to test (AFAICT, this is the problem with some platforms where SQLite found problems, the written data is cached somewhere distinct until it is written to disk, at which point the memory map will see it). I'll revise the test to close the file after writing, which I think is likely to imply something like fflush() but not imply anything like fsync().
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/40001
shess@chromium.org changed reviewers: + pvalenzuela@chromium.org
+pvalenzuela@ for the directory_backing_store_corruption_testing.cc change. https://codereview.chromium.org/1349863003/diff/40001/sync/test/directory_bac... File sync/test/directory_backing_store_corruption_testing.cc (right): https://codereview.chromium.org/1349863003/diff/40001/sync/test/directory_bac... sync/test/directory_backing_store_corruption_testing.cc:28: base::ScopedFILE db_file(base::OpenFile(backing_file_path, "rb+")); "wb" truncates the file then writes 4k of zeros. This is weird given the use of 32kb pages, but the specific problem was that it really shouldn't be manually manipulating a SQLite file while SQLite has it open. Overwriting rather than truncating should be less of a problem. BTW, sql::test::CorruptSizeInHeader() provides a should-always-work-right way to inject corruption into SQLite databases.
lgtm https://codereview.chromium.org/1349863003/diff/40001/sync/test/directory_bac... File sync/test/directory_backing_store_corruption_testing.cc (right): https://codereview.chromium.org/1349863003/diff/40001/sync/test/directory_bac... sync/test/directory_backing_store_corruption_testing.cc:28: base::ScopedFILE db_file(base::OpenFile(backing_file_path, "rb+")); On 2015/09/17 21:18:56, Scott Hess wrote: > "wb" truncates the file then writes 4k of zeros. This is weird given the use of > 32kb pages, but the specific problem was that it really shouldn't be manually > manipulating a SQLite file while SQLite has it open. Overwriting rather than > truncating should be less of a problem. > > BTW, sql::test::CorruptSizeInHeader() provides a should-always-work-right way to > inject corruption into SQLite databases. Ok. Thanks for making this change. If I understand you right, we should remove this method (CorruptDatabase) entirely and replace it with calls to sql::test::CorruptSizeInHeader()?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
On 2015/09/17 22:26:36, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) Oooooh... missing pragma are not errors, they just fail with no results.
https://codereview.chromium.org/1349863003/diff/40001/sync/test/directory_bac... File sync/test/directory_backing_store_corruption_testing.cc (right): https://codereview.chromium.org/1349863003/diff/40001/sync/test/directory_bac... sync/test/directory_backing_store_corruption_testing.cc:28: base::ScopedFILE db_file(base::OpenFile(backing_file_path, "rb+")); On 2015/09/17 21:41:05, pvalenzuela wrote: > On 2015/09/17 21:18:56, Scott Hess wrote: > > "wb" truncates the file then writes 4k of zeros. This is weird given the use > of > > 32kb pages, but the specific problem was that it really shouldn't be manually > > manipulating a SQLite file while SQLite has it open. Overwriting rather than > > truncating should be less of a problem. > > > > BTW, sql::test::CorruptSizeInHeader() provides a should-always-work-right way > to > > inject corruption into SQLite databases. > > Ok. Thanks for making this change. > > If I understand you right, we should remove this method (CorruptDatabase) > entirely and replace it with calls to sql::test::CorruptSizeInHeader()? Some of the code is using CorruptSizeInHeader() already, which makes me wonder/think that there's something up with this particular code. On iOS the test: DirectoryBackingStoreTest.CatastrophicErrorHandler_InvocationDuringSaveChanges is failing, now, and I'm not sure why. Possibly because iOS uses the system SQLite, which does not support memory-mapping in the first place, so maybe my change breaks POSIX-style while the previous version breaks mmap-style. If I had to guess, it's because the previous code was creating an entirely new file with different inode, which SQLite detects as part of locking under iOS, or probably a short read when built with our SQLite (we currently disable the file-move thing for stupid reasons). So with my change, under POSIX it's just using the in-memory buffer, which it assumes is valid because of exclusive mode, and thus never seeing the change. Probably that's why CorruptSizeInHeader() isn't being used, here, because this same issue would prevent it from working. I'll try to think of a way to tweak this to be more representative.
Still LGTM once you've fixed the iOS issues. https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:511: void Connection::ReleaseCacheMemoryIfNeeded(bool assume_changed) { On 2015/09/17 21:14:37, Scott Hess wrote: > On 2015/09/17 17:17:44, rmcilroy wrote: > > nit - /s/assume_changed/force_release/ ? > > I can see being against |assume_changed|. |force_release| isn't really it, > since it may not release if mmap not enabled, or if in a transaction (in that > case it will release later, though). > > Maybe |hint_changed|? Hmm, it's not really a hint though. How about implicit_change_performed or similar. Btw, why do the execute operations not update sqlite3_total_changes()? https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h File sql/connection.h (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h#newcod... sql/connection.h:695: int total_changes_; On 2015/09/17 21:14:37, Scott Hess wrote: > I changed this to |total_changes_last_release_| to discourage anyone from > assuming this is a generally-useful cache of sqlite3_total_changes(). How about total_changes_since_last_release_ for clarity?
I'm probably going to break off the sync testing change into a separate CL, as it has grown a new sql::test:: helper. https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:511: void Connection::ReleaseCacheMemoryIfNeeded(bool assume_changed) { On 2015/09/18 11:16:29, rmcilroy wrote: > Btw, why do the execute operations not update sqlite3_total_changes()? A statement like "UPDATE foo SET x=x+1 WHERE y<?" may or may not update any rows, and sqlite3_total_changes() will track that. But a statement like "CREATE TABLE foo (c TEXT)" won't update sqlite3_total_changes(), because no rows were changed. Likewise with "DROP TABLE", "ALTER TABLE", etc. DDL type statements are almost always passed to Execute(). Some DML statements could be, but they'd have to not reference any rows, like "DELETE FROM foo", but noticing their changes twice won't be a big deal. sqlite3_stmt_read_only() would be a reasonable alternative in this context to catch the DDL cases, but that would still leave things like PRAGMA, where answering the question is _really_ deep. Long-term, hopefully I can upstream a patch to expose this from SQLite, and it can make informed decisions at the pager layer.
https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:511: void Connection::ReleaseCacheMemoryIfNeeded(bool assume_changed) { On 2015/09/18 15:43:41, Scott Hess wrote: > On 2015/09/18 11:16:29, rmcilroy wrote: > > Btw, why do the execute operations not update sqlite3_total_changes()? > > A statement like "UPDATE foo SET x=x+1 WHERE y<?" may or may not update any > rows, and sqlite3_total_changes() will track that. But a statement like "CREATE > TABLE foo (c TEXT)" won't update sqlite3_total_changes(), because no rows were > changed. Likewise with "DROP TABLE", "ALTER TABLE", etc. > > DDL type statements are almost always passed to Execute(). Some DML statements > could be, but they'd have to not reference any rows, like "DELETE FROM foo", but > noticing their changes twice won't be a big deal. > > sqlite3_stmt_read_only() would be a reasonable alternative in this context to > catch the DDL cases, but that would still leave things like PRAGMA, where > answering the question is _really_ deep. > > Long-term, hopefully I can upstream a patch to expose this from SQLite, and it > can make informed decisions at the pager layer. SGTM, thanks for the explanation.
rebase
name changes, disable mmap for sync.
The CQ bit was checked by shess@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
overreverted sync fix, needed to keep iOS fix
The CQ bit was checked by shess@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
OK, I found that the sync issue is going to be a bit beyond this CL to deal with, so arranged to disable mmap for that db. I don't think that db is needed to see the broad results. https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.cc#newco... sql/connection.cc:511: void Connection::ReleaseCacheMemoryIfNeeded(bool assume_changed) { On 2015/09/18 11:16:29, rmcilroy wrote: > On 2015/09/17 21:14:37, Scott Hess wrote: > > On 2015/09/17 17:17:44, rmcilroy wrote: > > > nit - /s/assume_changed/force_release/ ? > > > > I can see being against |assume_changed|. |force_release| isn't really it, > > since it may not release if mmap not enabled, or if in a transaction (in that > > case it will release later, though). > > > > Maybe |hint_changed|? > > Hmm, it's not really a hint though. How about implicit_change_performed or > similar. Btw, why do the execute operations not update sqlite3_total_changes()? Acknowledged. https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h File sql/connection.h (right): https://codereview.chromium.org/1349863003/diff/20001/sql/connection.h#newcod... sql/connection.h:695: int total_changes_; On 2015/09/18 11:16:29, rmcilroy wrote: > On 2015/09/17 21:14:37, Scott Hess wrote: > > I changed this to |total_changes_last_release_| to discourage anyone from > > assuming this is a generally-useful cache of sqlite3_total_changes(). > > How about total_changes_since_last_release_ for clarity? total_changes_at_last_release_, as this is an absolute value. If I make it even longer, I guess that gives me incentive to get rid of it eventually!
The CQ bit was checked by shess@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, pvalenzuela@chromium.org Link to the patchset: https://codereview.chromium.org/1349863003/#ps100001 (title: "overreverted sync fix, needed to keep iOS fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
shess@chromium.org changed reviewers: + pavely@chromium.org
Nice, Patrick wasn't owner of the new part modified :-). pavely@ as owner for the bit in directory_backing_store.cc to disable mmap for this database. See bug 533682 for an overview of why it doesn't work out for testing.
ah, yeah. I am only owner in the test directory. Let me know if you need anything else for this CL. What impact, if any, will this have in non-test code? https://codereview.chromium.org/1349863003/diff/100001/sync/syncable/director... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/1349863003/diff/100001/sync/syncable/director... sync/syncable/directory_backing_store.cc:1694: // TODO(shess): Sync corruption tests interact poorly with mmap, disble for s/disble/disable/
Rebase
Speling chaneg.
On 2015/09/22 16:57:33, pvalenzuela wrote: > What impact, if any, will this have in non-test code? This will keep this database using the status quo. In theory, mmap mode should increase read performance and reduce memory overhead. https://codereview.chromium.org/1349863003/diff/100001/sync/syncable/director... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/1349863003/diff/100001/sync/syncable/director... sync/syncable/directory_backing_store.cc:1694: // TODO(shess): Sync corruption tests interact poorly with mmap, disble for On 2015/09/22 16:57:33, pvalenzuela wrote: > s/disble/disable/ Acknowledged.
btw, patch #6 looks clean except for the presubmit failure. So just waiting approval from pavely, I hope.
sync/ lgtm
The CQ bit was checked by shess@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, pvalenzuela@chromium.org Link to the patchset: https://codereview.chromium.org/1349863003/#ps140001 (title: "Speling chaneg.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/09/23 06:41:10, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) Thousands of lines of adb commands, but no diagnostic output at all, and the final result is purple, indicating a bot exception. Since I can't think of any reason this change should be platform-specific in this area, I'm just going to press the button again.
The CQ bit was checked by shess@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349863003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349863003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9a1948a4d6d445d5c8e209bdcd1cd050af72060b Cr-Commit-Position: refs/heads/master@{#350362}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1365783002/ by jbroman@chromium.org. The reason for reverting is: mmap_enabled_ isn't initialized, causing MSAN failures: https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_MSA... STDERR: ==3138==WARNING: MemorySanitizer: use-of-uninitialized-value STDERR: #0 0x7fc8068d3a65 in ReleaseCacheMemoryIfNeeded sql/connection.cc:513:7 STDERR: #1 0x7fc8068d3a65 in sql::Connection::ExecuteAndReturnErrorCode(char const*) sql/connection.cc:943:0 STDERR: #2 0x7fc8068ca454 in sql::Connection::OpenInternal(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, sql::Connection::Retry) sql/connection.cc:1275:9 STDERR: #3 0x7fc8068c845f in sql::Connection::Open(base::FilePath const&) sql/connection.cc:367:10 STDERR: #4 0x7fc806b1b868 in storage::QuotaDatabase::LazyOpen(bool) storage/browser/quota/quota_database.cc:488:14. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
