|
|
Created:
3 years, 10 months ago by Marijn Kruisselbrink Modified:
3 years, 10 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, kinuko+fileapi, nhiroki, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a use counter and histogram to FileReaderSync.
To hopefully eventually facilitate removing FileReaderSync from service
workers this adds both a use counter and histogram to log how often the
API is used in the various worker types.
BUG=688586
Review-Url: https://codereview.chromium.org/2682223006
Cr-Commit-Position: refs/heads/master@{#449833}
Committed: https://chromium.googlesource.com/chromium/src/+/77f740e334ba6fdb4d6493372e8009c544939ed4
Patch Set 1 #Patch Set 2 : add new histogram to histograms.xml #
Total comments: 6
Patch Set 3 : explicit #
Total comments: 9
Patch Set 4 : Reorder histogram enum #Patch Set 5 : clarify histogram description #
Messages
Total messages: 33 (23 generated)
The CQ bit was checked by mek@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 checked by mek@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...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by mek@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...
mek@chromium.org changed reviewers: + kinuko@chromium.org, mpearson@chromium.org
lgtm https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp (right): https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp:50: OTHER = 3, nit: as far as we follow the above comment values are guaranteed to be 0, 1, 2, ..., do we still want to be explicit? https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderSync.h (right): https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderSync.h:70: FileReaderSync(ExecutionContext*); nit: explicit https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1461: V8FileReaderSync_Constructor = 1808, nit: I think just using the class name as its prefix is more common (V8FileReaderSync -> FileReaderSync)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mek@chromium.org to run a CQ dry run
https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp (right): https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp:50: OTHER = 3, On 2017/02/10 at 00:41:38, kinuko wrote: > nit: as far as we follow the above comment values are guaranteed to be 0, 1, 2, ..., do we still want to be explicit? The documentation at https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... says yes, we do. https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderSync.h (right): https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderSync.h:70: FileReaderSync(ExecutionContext*); On 2017/02/10 at 00:41:38, kinuko wrote: > nit: explicit Done https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2682223006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1461: V8FileReaderSync_Constructor = 1808, On 2017/02/10 at 00:41:38, kinuko wrote: > nit: I think just using the class name as its prefix is more common (V8FileReaderSync -> FileReaderSync) V8FileReaderSync is what the "Measure" idl extended attribute ends up using. Anything else would require overriding it with MeasureAs
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp (right): https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp:50: OTHER = 3, OTHER sounds like a miscellaneous bucket. If so, please put that first. See (recently-added) last paragraph in this section: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1459: V8FileReaderSync_Constructor = 1808, Please resync and rebase. And also don't forget to run update_use_counter_feature_enum.py https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19450: + How often which worker types create FileReaderSync instances. how about? (how often -> Records; create -> creates) Records which worker types creates FileReaderSync instances. I assume this is recorded on every time a FileReaderSync variable is constructed. Can you please add a sentence here explaining roughly when that happens?
The CQ bit was checked by mek@chromium.org to run a CQ dry run
https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp (right): https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp:50: OTHER = 3, On 2017/02/10 at 22:56:42, Mark P wrote: > OTHER sounds like a miscellaneous bucket. If so, please put that first. See (recently-added) last paragraph in this section: > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Good point, done. https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2682223006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:1459: V8FileReaderSync_Constructor = 1808, On 2017/02/10 at 22:56:42, Mark P wrote: > Please resync and rebase. And also don't forget to run update_use_counter_feature_enum.py Yes? Of course I'll make sure the patch still applies before it lands? https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19450: + How often which worker types create FileReaderSync instances. On 2017/02/10 at 22:56:42, Mark P wrote: > how about? (how often -> Records; create -> creates) > Records which worker types creates FileReaderSync instances. I personally find that description a lot less clear... I'd like to at least keep it explicit that this counts how many instances of FileReaderSync are created. Also every histogram records something, so it seems kind of pointless to mention that in the description (okay, every histogram also records how often something happens, so maybe that's extraneous too, but I do believe that makes it clearer what is recorded). If you still think it needs rephrasing, maybe "For each FileReaderSync instance records the type of the worker that created the instance."? > I assume this is recorded on every time a FileReaderSync variable is constructed. Can you please add a sentence here explaining roughly when that happens? Not sure what you're asking me to do? A FileReaderSync instances is constructed whenever a FileReaderSync instance is created. It's not like there is any kind of explanation for when websites do that.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm modulo nits --mark https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19450: + How often which worker types create FileReaderSync instances. On 2017/02/10 23:17:28, Marijn Kruisselbrink wrote: > On 2017/02/10 at 22:56:42, Mark P wrote: > > how about? (how often -> Records; create -> creates) > > Records which worker types creates FileReaderSync instances. > > I personally find that description a lot less clear... I'd like to at least keep > it explicit that this counts how many instances of FileReaderSync are created. > Also every histogram records something, so it seems kind of pointless to mention > that in the description (okay, every histogram also records how often something > happens, so maybe that's extraneous too, but I do believe that makes it clearer > what is recorded). > > If you still think it needs rephrasing, maybe "For each FileReaderSync instance > records the type of the worker that created the instance."? I like that a lot better. The reason for my original objection is that starting a histogram description off with "How often" or "how many" makes readers think it's going to be a count histogram, where the value emitted is a number, not an enum histogram. Thus, I'd like to avoid that original phrasing. > > > I assume this is recorded on every time a FileReaderSync variable is > constructed. Can you please add a sentence here explaining roughly when that > happens? > > Not sure what you're asking me to do? A FileReaderSync instances is constructed > whenever a FileReaderSync instance is created. It's not like there is any kind > of explanation for when websites do that. Well, you gave me a partial answer: it's *web sites* that create an instance. I didn't know that before from the description. Can each website create more than one instance, or is there roughly at most one instance ever associated with a web site at one time? If they create more than one, this is common, e.g., are web sites that use this API creating many on each page load? Basically, I'm looking for a rough idea of when / how often these things are expected to be created.
The CQ bit was checked by mek@chromium.org to run a CQ dry run
https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19450: + How often which worker types create FileReaderSync instances. On 2017/02/10 at 23:42:35, Mark P wrote: > On 2017/02/10 23:17:28, Marijn Kruisselbrink wrote: > > On 2017/02/10 at 22:56:42, Mark P wrote: > > > how about? (how often -> Records; create -> creates) > > > Records which worker types creates FileReaderSync instances. > > > > I personally find that description a lot less clear... I'd like to at least keep > > it explicit that this counts how many instances of FileReaderSync are created. > > Also every histogram records something, so it seems kind of pointless to mention > > that in the description (okay, every histogram also records how often something > > happens, so maybe that's extraneous too, but I do believe that makes it clearer > > what is recorded). > > > > If you still think it needs rephrasing, maybe "For each FileReaderSync instance > > records the type of the worker that created the instance."? > > I like that a lot better. The reason for my original objection is that starting a histogram description off with "How often" or "how many" makes readers think it's going to be a count histogram, where the value emitted is a number, not an enum histogram. Thus, I'd like to avoid that original phrasing. Okay, makes sense. Changed description. > > > I assume this is recorded on every time a FileReaderSync variable is > > constructed. Can you please add a sentence here explaining roughly when that > > happens? > > > > Not sure what you're asking me to do? A FileReaderSync instances is constructed > > whenever a FileReaderSync instance is created. It's not like there is any kind > > of explanation for when websites do that. > > Well, you gave me a partial answer: it's *web sites* that create an instance. I didn't know that before from the description. Can each website create more than one instance, or is there roughly at most one instance ever associated with a web site at one time? If they create more than one, this is common, e.g., are web sites that use this API creating many on each page load? Basically, I'm looking for a rough idea of when / how often these things are expected to be created. Basically, we have no idea what websites are doing with this API. The point in adding the histogram is to try to get such an idea (and more specifically, ideally the use counter would be enough, but I'd like to backport this to M57, in which the use counters aren't and won't be able to collect data from most workers, which is the only place this web API is exposed). I changed "workers" to "web workers" in the description, to hopefully make it clearer that it's websites using this. Btw, was the histogram guidelines doc ever shared on chromium-dev? Seems like that could be useful, especially as it has a bunch of advise/policies that as far as I know weren't strongly enforced (or certainly not documented) earlier. Making sure people know about the policies could probably help make reviews go more smoothly...
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 mek@chromium.org
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2682223006/#ps100001 (title: "clarify histogram description")
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": 100001, "attempt_start_ts": 1486774452205090, "parent_rev": "e23eed07b41b99be905bffbbcadb0d761c40eb7d", "commit_rev": "77f740e334ba6fdb4d6493372e8009c544939ed4"}
Message was sent while issue was closed.
Description was changed from ========== Add a use counter and histogram to FileReaderSync. To hopefully eventually facilitate removing FileReaderSync from service workers this adds both a use counter and histogram to log how often the API is used in the various worker types. BUG=688586 ========== to ========== Add a use counter and histogram to FileReaderSync. To hopefully eventually facilitate removing FileReaderSync from service workers this adds both a use counter and histogram to log how often the API is used in the various worker types. BUG=688586 Review-Url: https://codereview.chromium.org/2682223006 Cr-Commit-Position: refs/heads/master@{#449833} Committed: https://chromium.googlesource.com/chromium/src/+/77f740e334ba6fdb4d6493372e80... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/77f740e334ba6fdb4d6493372e80...
Message was sent while issue was closed.
https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2682223006/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19450: + How often which worker types create FileReaderSync instances. > Btw, was the histogram guidelines doc ever shared on chromium-dev? Seems like > that could be useful, especially as it has a bunch of advise/policies that as > far as I know weren't strongly enforced (or certainly not documented) earlier. > Making sure people know about the policies could probably help make reviews go > more smoothly... No, it wasn't. That's a good idea. Thanks! I'll do it sometime this month. |