Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727103003:
[sql] Histograms for I/O calls seen by browser VFS. (Closed)
|
Created:
3 years, 9 months ago by Scott Hess - ex-Googler Modified:
3 years, 9 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sql] Histograms for I/O calls seen by browser VFS.
Track the common VFS calls which do disk I/O, to help analyze the impact
of any changes to how database connections are used or setup.
BUG=698010
Review-Url: https://codereview.chromium.org/2727103003
Cr-Commit-Position: refs/heads/master@{#457099}
Committed: https://chromium.googlesource.com/chromium/src/+/3998fbc50bb215733a081780b286804aa18225c3
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Use histogram suffixes. #
Dependent Patchsets: Messages
Total messages: 28 (14 generated)
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...
shess@chromium.org changed reviewers: + mariakhomenko@chromium.org, pasko@chromium.org
Sorry, I got distracted. Better late than never. I think this CL is probably worthwhile regardless of where this experiment goes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Rebase
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
shess@chromium.org changed reviewers: + jwd@chromium.org
jwd@ for histograms.xml. The events histogram will have some buckets less, some more, they won't really sum to a useful value, but they will be useful to compare across experiments or other changes to the database library. They also should provide interesting correlations with Sqlite.Stats . With the read/write/fetch items, I want to see both the number of events plus the magnitude of the events. So they will somewhat overlap with the events enum, but add a little side data. I can "prove" that the items in these histograms should have strong quantum lines, but I'm not sure how to figure out the distribution across different databases (which can have different sizes), so I think it's possible that the histograms will not be necessary in the long term. I'm thinking of evaluating the results on canary/dev to see if they provide anything actionable, or if they can be approximated by comments alongside the VFS_IO_* events.
https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66710: +<histogram name="Sqlite.VfsFetch" units="bytes"> Can you reformulate the Fetch, Read, and Write histograms to use histogram suffixes. Documentation about it is at the top of the file.
https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66710: +<histogram name="Sqlite.VfsFetch" units="bytes"> On 2017/03/07 22:33:57, jwd wrote: > Can you reformulate the Fetch, Read, and Write histograms to use histogram > suffixes. Documentation about it is at the top of the file. Sorry, rework them in what way? I can't really integrate them into the existing Sqlite.X.* suffix system, because the vfs itself is a singleton. Though I suppose I could write up a pragma or something to pass a string down for customization purposes, if it becomes necessary.
https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66710: +<histogram name="Sqlite.VfsFetch" units="bytes"> On 2017/03/07 23:02:06, Scott Hess wrote: > On 2017/03/07 22:33:57, jwd wrote: > > Can you reformulate the Fetch, Read, and Write histograms to use histogram > > suffixes. Documentation about it is at the top of the file. > > Sorry, rework them in what way? > > I can't really integrate them into the existing Sqlite.X.* suffix system, > because the vfs itself is a singleton. Though I suppose I could write up a > pragma or something to pass a string down for customization purposes, if it > becomes necessary. I think what jwd@ is suggesting is something like this: <histogram name="Sqlite.Vfs"> ... </histogram> <histogram_suffixes name="VfsSuffixes"> <suffix name="Events" label="events"/> <suffix name="Fetch" ../> <suffix name="Read" .. /> <suffix name="Write" ../> <affected-histogram name="Sqlite.Vfs"/> </histogram_suffixes> And then in your code you'd just pass Sqlite.Vfs_Read as the name of the histogram instead of Sqlite.VfsRead
https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66710: +<histogram name="Sqlite.VfsFetch" units="bytes"> On 2017/03/08 06:27:38, Maria wrote: > On 2017/03/07 23:02:06, Scott Hess wrote: > > On 2017/03/07 22:33:57, jwd wrote: > > > Can you reformulate the Fetch, Read, and Write histograms to use histogram > > > suffixes. Documentation about it is at the top of the file. > > > > Sorry, rework them in what way? > > > > I can't really integrate them into the existing Sqlite.X.* suffix system, > > because the vfs itself is a singleton. Though I suppose I could write up a > > pragma or something to pass a string down for customization purposes, if it > > becomes necessary. > > I think what jwd@ is suggesting is something like this: > > <histogram name="Sqlite.Vfs"> > ... > </histogram> > > <histogram_suffixes name="VfsSuffixes"> > <suffix name="Events" label="events"/> > <suffix name="Fetch" ../> > <suffix name="Read" .. /> > <suffix name="Write" ../> > <affected-histogram name="Sqlite.Vfs"/> > </histogram_suffixes> > > And then in your code you'd just pass Sqlite.Vfs_Read as the name of the > histogram instead of Sqlite.VfsRead Yes exactly, I was just suggestion a change to the histograms.xml part.
Use histogram suffixes.
On 2017/03/08 15:37:47, jwd wrote: > https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2727103003/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:66710: +<histogram > name="Sqlite.VfsFetch" units="bytes"> > On 2017/03/08 06:27:38, Maria wrote: > > On 2017/03/07 23:02:06, Scott Hess wrote: > > > On 2017/03/07 22:33:57, jwd wrote: > > > > Can you reformulate the Fetch, Read, and Write histograms to use histogram > > > > suffixes. Documentation about it is at the top of the file. > > > > > > Sorry, rework them in what way? > > > > > > I can't really integrate them into the existing Sqlite.X.* suffix system, > > > because the vfs itself is a singleton. Though I suppose I could write up a > > > pragma or something to pass a string down for customization purposes, if it > > > becomes necessary. > > > > I think what jwd@ is suggesting is something like this: > > > > <histogram name="Sqlite.Vfs"> > > ... > > </histogram> > > > > <histogram_suffixes name="VfsSuffixes"> > > <suffix name="Events" label="events"/> > > <suffix name="Fetch" ../> > > <suffix name="Read" .. /> > > <suffix name="Write" ../> > > <affected-histogram name="Sqlite.Vfs"/> > > </histogram_suffixes> > > > > And then in your code you'd just pass Sqlite.Vfs_Read as the name of the > > histogram instead of Sqlite.VfsRead > > Yes exactly, I was just suggestion a change to the histograms.xml part. Ok, I guess. Sqlite.VfsEvents isn't the same kind as the other three, so I left it separate (unless there's something I'm missing there to let me mix different types under histogram_suffixes), but renamed it to have the same "look". Now I'm pondering s/Vfs/VFS/, since it's an acronym (Virtual File System), but URL is already laughably inconsistent, so...
lgtm
The CQ bit was checked by shess@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2727103003/#ps40001 (title: "Use histogram suffixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489591334522600,
"parent_rev": "9236e4f318ac01aa285718353c64c90521b0255f", "commit_rev":
"3998fbc50bb215733a081780b286804aa18225c3"}
Message was sent while issue was closed.
Description was changed from ========== [sql] Histograms for I/O calls seen by browser VFS. Track the common VFS calls which do disk I/O, to help analyze the impact of any changes to how database connections are used or setup. BUG=698010 ========== to ========== [sql] Histograms for I/O calls seen by browser VFS. Track the common VFS calls which do disk I/O, to help analyze the impact of any changes to how database connections are used or setup. BUG=698010 Review-Url: https://codereview.chromium.org/2727103003 Cr-Commit-Position: refs/heads/master@{#457099} Committed: https://chromium.googlesource.com/chromium/src/+/3998fbc50bb215733a081780b286... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3998fbc50bb215733a081780b286... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
