Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(640)

Issue 1533703002: [sql] Consider fresh databases suitable for memory-mapped I/O. (Closed)

Created:
5 years ago by Scott Hess - ex-Googler
Modified:
5 years ago
Reviewers:
rmcilroy
CC:
chromium-reviews, tim+watch_chromium.org, extensions-reviews_chromium.org, felt, zea+watch_chromium.org, pvalenzuela+watch_chromium.org, shishir+watch_chromium.org, maxbogue+watch_chromium.org, jam, plaree+watch_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[sql] Consider fresh databases suitable for memory-mapped I/O. GetAppropriateMmapSize() is called to validate the database as error-free before enabling memory-mapped mode. The validation status is stored in the [meta] table, since new databases have no [meta] table it previously defaulted to not turning on memory-mapping. Then on second open the file would be validated. This change uses the lack of [meta] table as a signal that the file is new and should be safe to memory-map. Additionally the code creating [meta] pre-populates that setting. Additionally disable memory-mapped mode for databases which do not make use of the [meta] table. This makes the existing settings explicit. BUG=537742, 555578 TBR=rdevlin.cronin@chromium.org, michaeln@chromium.org, zea@chromium.org, sky@chromium.org Committed: https://crrev.com/9bf2c6745727731429baa207db994de5278505a0 Cr-Commit-Position: refs/heads/master@{#365959}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Derp, don't need to disable again. #

Patch Set 3 : Fixes for iOS and mojo #

Patch Set 4 : eyeroll and 0UL #

Unified diffs Side-by-side diffs Delta from patch set Stats (+187 lines, -49 lines) Patch
M chrome/browser/extensions/activity_log/activity_database.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/predictors/predictor_database.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/dom_storage/dom_storage_database.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M sql/connection.h View 1 chunk +1 line, -0 lines 0 comments Download
M sql/connection.cc View 6 chunks +33 lines, -46 lines 0 comments Download
M sql/connection_unittest.cc View 1 2 3 1 chunk +87 lines, -0 lines 0 comments Download
M sql/meta_table.h View 2 chunks +15 lines, -0 lines 0 comments Download
M sql/meta_table.cc View 3 chunks +35 lines, -1 line 0 comments Download
M sync/syncable/directory_backing_store.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download

Messages

Total messages: 30 (14 generated)
Scott Hess - ex-Googler
I've been meaning to land something in here for awhile, but I am _not_ at ...
5 years ago (2015-12-16 23:09:08 UTC) #2
rmcilroy
LGTM https://codereview.chromium.org/1533703002/diff/1/sync/syncable/directory_backing_store.cc File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/1533703002/diff/1/sync/syncable/directory_backing_store.cc#newcode1737 sync/syncable/directory_backing_store.cc:1737: db_->set_mmap_disabled(); You've already disabled it here - just ...
5 years ago (2015-12-17 10:03:22 UTC) #4
Scott Hess - ex-Googler
Derp, don't need to disable again.
5 years ago (2015-12-17 19:25:41 UTC) #5
Scott Hess - ex-Googler
Fixes for iOS and mojo
5 years ago (2015-12-17 21:49:52 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1533703002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1533703002/40001
5 years ago (2015-12-17 22:07:28 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/130130) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, ...
5 years ago (2015-12-17 22:16:19 UTC) #11
Scott Hess - ex-Googler
eyeroll and 0UL
5 years ago (2015-12-17 22:26:13 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1533703002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1533703002/60001
5 years ago (2015-12-17 22:35:28 UTC) #14
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/94324) win_chromium_x64_rel_ng on ...
5 years ago (2015-12-17 23:49:29 UTC) #16
Scott Hess - ex-Googler
On 2015/12/17 23:49:29, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...
5 years ago (2015-12-18 00:00:32 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1533703002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1533703002/60001
5 years ago (2015-12-18 00:01:24 UTC) #20
Scott Hess - ex-Googler
TBR for the set_mmap_disabled() additions: rdevlin.cronin@ for activity_database.cc michaeln@ for dom_storage_database.cc zea@ for directory_backing_store.cc sky@ ...
5 years ago (2015-12-18 00:12:47 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1533703002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1533703002/60001
5 years ago (2015-12-18 00:34:29 UTC) #25
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years ago (2015-12-18 01:18:15 UTC) #27
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/9bf2c6745727731429baa207db994de5278505a0 Cr-Commit-Position: refs/heads/master@{#365959}
5 years ago (2015-12-18 01:19:59 UTC) #29
Scott Hess - ex-Googler
5 years ago (2015-12-18 21:45:56 UTC) #30
Message was sent while issue was closed.
I'll be OOT until 12/26 or /27.  So if this needs to be reverted, you have my
permission.  It mostly addresses first-start situations in perf testing, and
AFAICT the metrics it was supposed to improve did improve.  I don't think it
should enable any new functionality on canary/dev, so if there's a wicked new
crasher or something, this is probably not it (unless it's in the specific
code).

Powered by Google App Engine
This is Rietveld 408576698