Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728543008:
[sql] Experiment to enable smart-vacuum on Android.
|
Created:
3 years, 9 months ago by Scott Hess - ex-Googler Modified:
3 years, 8 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[sql] Experiment to enable smart-vacuum on Android.
BUG=698010
Patch Set 1 #Patch Set 2 : cl feature suggestion, and test it. #
Total comments: 6
Patch Set 3 : Just read page size. #
Depends on Patchset: Messages
Total messages: 26 (13 generated)
shess@chromium.org changed reviewers: + mariakhomenko@chromium.org, pasko@chromium.org
I was thinking "Yeah, experiments seem so easy these days", then as I was working on this I realized that the places I had seen experiments were all in components/. I am not actually sure where to hook this from. So if you have ideas, or would like to take the CL over, let me know.
On 2017/03/03 00:13:17, Scott Hess wrote: > I was thinking "Yeah, experiments seem so easy these days", then as I was > working on this I realized that the places I had seen experiments were all in > components/. I am not actually sure where to hook this from. So if you have > ideas, or would like to take the CL over, let me know. So, I think it would need to be something like what this undoes: https://codereview.chromium.org/1927443002/ but without more generally applicable and hooked into the experiment sub-system. The main reason I'm thinking "experiment" is so that we can easily compare/contrast the histogram feedback without having to do a lot of work to analyze things. Does that make any sense?
On 2017/03/03 19:47:04, Scott Hess wrote: > On 2017/03/03 00:13:17, Scott Hess wrote: > > I was thinking "Yeah, experiments seem so easy these days", then as I was > > working on this I realized that the places I had seen experiments were all in > > components/. I am not actually sure where to hook this from. So if you have > > ideas, or would like to take the CL over, let me know. > > So, I think it would need to be something like what this undoes: > https://codereview.chromium.org/1927443002/ > but without more generally applicable and hooked into the experiment sub-system. > > The main reason I'm thinking "experiment" is so that we can easily > compare/contrast the histogram feedback without having to do a lot of work to > analyze things. Does that make any sense? I think the right way to do this is to add const base::Feature kSqliteAutoVacuumEnabled { "SqliteAutoVacuum", base::FEATURE_DISABLED_BY_DEFAULT}; to sql/connection.cc You can get rid of the public interface entirely, and just check whether the feature is on in the code by doing base::FeatureList::IsEnabled(kSqliteAutoVacuumEnabled); Then in Finch we can configure this to be android experiment only.
On 2017/03/03 20:02:01, Maria wrote: > On 2017/03/03 19:47:04, Scott Hess wrote: > > On 2017/03/03 00:13:17, Scott Hess wrote: > > > I was thinking "Yeah, experiments seem so easy these days", then as I was > > > working on this I realized that the places I had seen experiments were all > in > > > components/. I am not actually sure where to hook this from. So if you > have > > > ideas, or would like to take the CL over, let me know. > > > > So, I think it would need to be something like what this undoes: > > https://codereview.chromium.org/1927443002/ > > but without more generally applicable and hooked into the experiment > sub-system. > > > > The main reason I'm thinking "experiment" is so that we can easily > > compare/contrast the histogram feedback without having to do a lot of work to > > analyze things. Does that make any sense? > > I think the right way to do this is to add > > const base::Feature kSqliteAutoVacuumEnabled { > "SqliteAutoVacuum", base::FEATURE_DISABLED_BY_DEFAULT}; > > to sql/connection.cc > > You can get rid of the public interface entirely, and just check whether the > feature is on in the code by doing > base::FeatureList::IsEnabled(kSqliteAutoVacuumEnabled); > > Then in Finch we can configure this to be android experiment only. +1, that's how I'd attempt to do it, plus probably a parameter to base::Feature to do more/less slack. By the way, what is the intuition around slack for chunk_size != 4KiB? I totally lack my intuition here.
cl feature suggestion, and test 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/v2/patch-status/codereview.chromium.or...
On 2017/03/06 17:13:23, pasko wrote: > On 2017/03/03 20:02:01, Maria wrote: > > On 2017/03/03 19:47:04, Scott Hess wrote: > > > On 2017/03/03 00:13:17, Scott Hess wrote: > > > > I was thinking "Yeah, experiments seem so easy these days", then as I was > > > > working on this I realized that the places I had seen experiments were all > > in > > > > components/. I am not actually sure where to hook this from. So if you > > have > > > > ideas, or would like to take the CL over, let me know. > > > > > > So, I think it would need to be something like what this undoes: > > > https://codereview.chromium.org/1927443002/ > > > but without more generally applicable and hooked into the experiment > > sub-system. > > > > > > The main reason I'm thinking "experiment" is so that we can easily > > > compare/contrast the histogram feedback without having to do a lot of work > to > > > analyze things. Does that make any sense? > > > > I think the right way to do this is to add > > > > const base::Feature kSqliteAutoVacuumEnabled { > > "SqliteAutoVacuum", base::FEATURE_DISABLED_BY_DEFAULT}; > > > > to sql/connection.cc > > > > You can get rid of the public interface entirely, and just check whether the > > feature is on in the code by doing > > base::FeatureList::IsEnabled(kSqliteAutoVacuumEnabled); > > > > Then in Finch we can configure this to be android experiment only. > > +1, that's how I'd attempt to do it, plus probably a parameter to base::Feature > to do more/less slack. > > By the way, what is the intuition around slack for chunk_size != 4KiB? I totally > lack my intuition here. Sorry for the delay - I've been trying to get some other fish fried. Your suggestions seem like a better idea than what I found myself, so I guess PTAL. I copied the feature description because exposing it through the sql::Connection interface felt atrocious because it would pull in base/feature_list.h (which is presumably why some other cases seem to use a dedicated feature header, maybe?). Does this need a FEATURE_VALUE_TYPE() type entry somewhere to inform chrome or components about it, or does it "just work"? WRT the chunk_size, I rewrote the code a little. Previously, it was so that I could have a single #ifdef without having to dance around to get the existing code (selecting the chunk size) to match the new pragma. With the rewrite, I don't think the problem comes up. Overall, the idea was that for a small file, we value compactness, so don't set chunking. For large files, we set 32k chunking, for files in between we set 4k chunking. Those values weren't really informed by much of anything, I just picked "seems good enough" values to keep the approximate "wasted" pages a relatively small portion of the total file size.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2728543008/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2728543008/diff/20001/sql/connection.cc#newco... sql/connection.cc:1868: const int page_size = page_size_ ? page_size_ : 1024; The 1024 constant here for page_size, is this just for the unit test? To me it's confusing that you say that the default page size is 4k, but then set a const default to 1k. https://codereview.chromium.org/2728543008/diff/20001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): https://codereview.chromium.org/2728543008/diff/20001/sql/connection_unittest... sql/connection_unittest.cc:1669: base::FEATURE_DISABLED_BY_DEFAULT}; This line in test should not do anything -- e.g. it would define the feature, but not enabled it. There's base::test::ScopedFeatureList to help with enabling features in test.
https://codereview.chromium.org/2728543008/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2728543008/diff/20001/sql/connection.cc#newco... sql/connection.cc:1868: const int page_size = page_size_ ? page_size_ : 1024; On 2017/03/28 22:14:59, Maria wrote: > The 1024 constant here for page_size, is this just for the unit test? To me it's > confusing that you say that the default page size is 4k, but then set a const > default to 1k. Maybe I'll elaborate the comment. SQLite's default page size is 4096, except for platforms using system SQLite (iOS, maybe Chromium on Linux), except if you're linking with an older SQLite (or if I have to revert the import), etc. I could write code to calculate the actual page size, but I believe all of our clients actually set an explicit page size, so it wouldn't really help in practice, so I figure it's reasonable to land that code later. Maybe I'll just proto the code in the morning, perhaps it's easier than updating the comment. https://codereview.chromium.org/2728543008/diff/20001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): https://codereview.chromium.org/2728543008/diff/20001/sql/connection_unittest... sql/connection_unittest.cc:1669: base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/03/28 22:14:59, Maria wrote: > This line in test should not do anything -- e.g. it would define the feature, > but not enabled it. > > There's base::test::ScopedFeatureList to help with enabling features in test. This line doesn't do anything, but it's used in the ScopredFeatureList::InitAnd*Feature() calls below. AFAICT, I need a base::Feature (or FeatureList) to pass to any of those calls, whether to enable or disable.
https://codereview.chromium.org/2728543008/diff/20001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): https://codereview.chromium.org/2728543008/diff/20001/sql/connection_unittest... sql/connection_unittest.cc:1669: base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/03/29 03:28:46, Scott Hess wrote: > On 2017/03/28 22:14:59, Maria wrote: > > This line in test should not do anything -- e.g. it would define the feature, > > but not enabled it. > > > > There's base::test::ScopedFeatureList to help with enabling features in test. > > This line doesn't do anything, but it's used in the > ScopredFeatureList::InitAnd*Feature() calls below. AFAICT, I need a > base::Feature (or FeatureList) to pass to any of those calls, whether to enable > or disable. Um, sorry about this comment. I somehow completely missed that you are using ScopedFeatureList below using this variable, even though I was looking for it.
Just read page size.
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/v2/patch-status/codereview.chromium.or...
Also, the previous pass' broken bots were ... random chance? I don't know, they didn't seem related. https://codereview.chromium.org/2728543008/diff/20001/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2728543008/diff/20001/sql/connection.cc#newco... sql/connection.cc:1868: const int page_size = page_size_ ? page_size_ : 1024; On 2017/03/29 03:28:46, Scott Hess wrote: > On 2017/03/28 22:14:59, Maria wrote: > > The 1024 constant here for page_size, is this just for the unit test? To me > it's > > confusing that you say that the default page size is 4k, but then set a const > > default to 1k. > > Maybe I'll elaborate the comment. SQLite's default page size is 4096, except > for platforms using system SQLite (iOS, maybe Chromium on Linux), except if > you're linking with an older SQLite (or if I have to revert the import), etc. I > could write code to calculate the actual page size, but I believe all of our > clients actually set an explicit page size, so it wouldn't really help in > practice, so I figure it's reasonable to land that code later. > > Maybe I'll just proto the code in the morning, perhaps it's easier than updating > the comment. Bah, I just went with fetching the page-size from the db. I am honestly not sure if this is something that should be more broadly exposed or not, so I kept it local.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/29 21:30:45, Maria wrote: > lgtm Unfortunately, sql::Connection::Open() is called before the system has initialized feature lists in many cases, which causes a CHECK failure: FATAL:feature_list.cc(250)] Check failed: !g_initialized_from_accessor. I do not fully understand what this means. I _think_ it means that there needs to be a particular ordering between first access to the feature list and first use. It's possible this is a test-only failure, like there's some non-obvious set of new setup dependencies which my change needs someone else to have before it can work. I'm not sure that problem is worth solving, given that this only applies to Android. But I don't think an #if to make the change Android-specific will solve the build problem. I'm out next week, and I'm running out of runway to even do anything with it, so if either of you want to run with this to start generating data, you have my blessing and encouragement. [Actually, if you want to see results from this, it might be best to move on it from your end.] The operation of SQLITE_FCNTL_CHUNK_SIZE reaches down to the VFS layer of SQLite, while the auto_vacuum_slack_pages PRAGMA affects the pager layer, which is why the setup code is a little funky, because it has to bridge between the native units of each. It may be that there is a way to link them within SQLite, I was just trying to keep the patch minimal (I'd rather have 12 lines of code up in sql::Connection::Open() than a routinely-broken patch against SQLite). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
