Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499263002:
Add UMA to estimate deroppable memory usage of encoded data size in Resources (Closed)
|
Created:
4 years, 1 month ago by hajimehoshi Modified:
4 years, 1 month ago CC:
chromium-reviews, tyoshino+watch_chromium.org, blink-reviews-style_chromium.org, Yoav Weiss, gavinp+loader_chromium.org, asvitkine+watch_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA to estimate deroppable memory usage of encoded data size in Resources
This CL adds a UMA to estimate droppable memory usage of encoded data
size in Resources when reusing disk cache is implemented. If it is 100%
sure that an encoded data is in disk cache, it is possible to remove
the same data from memory to reduce memory usage, and the UMA measures
this effectiveness.
BUG=664437
Committed: https://crrev.com/bd79bd92221f5057521d62639b7c5e84645032ef
Cr-Commit-Position: refs/heads/master@{#433491}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address on reviews #
Total comments: 5
Patch Set 3 : (rebase) #Patch Set 4 : Address on yhirano's review #
Total comments: 2
Patch Set 5 : Use platform/Histgram.h instead of adding depencency from core to base #
Depends on Patchset: Messages
Total messages: 40 (18 generated)
hajimehoshi@chromium.org changed reviewers: + haraken@chromium.org, isherman@chromium.org, yhirano@chromium.org
PTAL
https://codereview.chromium.org/2499263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2499263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:405: bool isReloadable() const; What does this method mean?
https://codereview.chromium.org/2499263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2499263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:405: bool isReloadable() const; On 2016/11/15 10:21:55, yhirano wrote: > What does this method mean? This returns true when the resource can be reloaded. For example, if the image is used as CSS background, it is difficult to load the encoded data from the disk cache in the current implementation, and this function returns false in this case.
https://codereview.chromium.org/2499263002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2499263002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:25922: +<histogram name="Memory.Renderer.EstimantedDroppableEncodedSize" units="KB"> nit: s/Estimanted/Estimated (please double-check the name in the C++ code as well) https://codereview.chromium.org/2499263002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:25928: + crbug/664437. Please document when this metrics is recorded.
The CQ bit was checked by hajimehoshi@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...
Thank you! BTW, what's the difference between tools/histograms/histograms.xml and tools/metrics/histograms/histograms.xml? https://codereview.chromium.org/2499263002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2499263002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:25922: +<histogram name="Memory.Renderer.EstimantedDroppableEncodedSize" units="KB"> On 2016/11/16 06:33:35, Ilya Sherman wrote: > nit: s/Estimanted/Estimated (please double-check the name in the C++ code as > well) Done. https://codereview.chromium.org/2499263002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:25928: + crbug/664437. On 2016/11/16 06:33:35, Ilya Sherman wrote: > Please document when this metrics is recorded. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.h (right): https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.h:221: bool m_isRefetchableFromDiskCache; How about having a function that turns this flag off and calling the function from the StylefetchedImage constructor? https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.h:221: bool m_isRefetchableFromDiskCache; I think including "data" or "body" in the name is a good idea. m_isAllowedToRefetchData[FromDiskCache], for example? Anyway, please write some comments.
Patchset #3 (id:40001) has been deleted
Thank you! https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.h (right): https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.h:221: bool m_isRefetchableFromDiskCache; On 2016/11/16 11:25:38, yhirano wrote: > I think including "data" or "body" in the name is a good idea. > m_isAllowedToRefetchData[FromDiskCache], for example? > > Anyway, please write some comments. > Done. (setNotRefetchableDataFromDiskCache seems weird. Do you have any suggestion?)
The CQ bit was checked by hajimehoshi@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...
https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.h (right): https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.h:221: bool m_isRefetchableFromDiskCache; On 2016/11/16 12:15:48, hajimehoshi wrote: > On 2016/11/16 11:25:38, yhirano wrote: > > I think including "data" or "body" in the name is a good idea. > > m_isAllowedToRefetchData[FromDiskCache], for example? > > > > Anyway, please write some comments. > > > > Done. (setNotRefetchableDataFromDiskCache seems weird. Do you have any > suggestion?) I mean the name setNotRefetchableDataFromDiskCache is weird but I couldn't come up with a better name...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics LGTM, thanks. On 2016/11/16 09:59:10, hajimehoshi wrote: > BTW, what's the difference between tools/histograms/histograms.xml and > tools/metrics/histograms/histograms.xml? tools/histograms/histograms.xml was previously part of src-internal, but is long obsolete. I'd recommend unmapping it in your local checkout.
lgtm https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.h (right): https://codereview.chromium.org/2499263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.h:221: bool m_isRefetchableFromDiskCache; On 2016/11/16 12:19:48, hajimehoshi wrote: > On 2016/11/16 12:15:48, hajimehoshi wrote: > > On 2016/11/16 11:25:38, yhirano wrote: > > > I think including "data" or "body" in the name is a good idea. > > > m_isAllowedToRefetchData[FromDiskCache], for example? > > > > > > Anyway, please write some comments. > > > > > > > Done. (setNotRefetchableDataFromDiskCache seems weird. Do you have any > > suggestion?) > > I mean the name setNotRefetchableDataFromDiskCache is weird but I couldn't come > up with a better name... Is it better to focus on dropping / discarding the encoded data rather than refeching in the field name? After all that is what you want to do. But it's up to you. https://codereview.chromium.org/2499263002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/DEPS (right): https://codereview.chromium.org/2499263002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/DEPS:2: "+base/metrics/histogram_macros.h", I don't know if core can depend on base. I'm happy to defer to haraken@ on this point.
Sorry for the review delay. However, I don't quite understand why you want to measure this. What we want to measure is NOT how much encoded images are droppable with the current disk cache but how much encoded images are droppable when we implement the blob-style API on the disk cache (i.e., how much encoded images are droppable when we guarantee all original resources are kept alive in the disk cache), right? In other words, what we want to measure is just the amount of memory of all encoded images.
On 2016/11/17 05:06:28, haraken wrote: > Sorry for the review delay. > > However, I don't quite understand why you want to measure this. What we want to > measure is NOT how much encoded images are droppable with the current disk cache > but how much encoded images are droppable when we implement the blob-style API > on the disk cache (i.e., how much encoded images are droppable when we guarantee > all original resources are kept alive in the disk cache), right? I don't understand what's the difference between them. I intended this CL measures how much encoded images are droppable when the blob-style API is implemented. > In other words, what we want to measure is just the amount of memory of all > encoded images. You mean we should measure the total amount?
On 2016/11/17 05:32:28, hajimehoshi wrote: > On 2016/11/17 05:06:28, haraken wrote: > > Sorry for the review delay. > > > > However, I don't quite understand why you want to measure this. What we want > to > > measure is NOT how much encoded images are droppable with the current disk > cache > > but how much encoded images are droppable when we implement the blob-style API > > on the disk cache (i.e., how much encoded images are droppable when we > guarantee > > all original resources are kept alive in the disk cache), right? > > I don't understand what's the difference between them. I intended this CL > measures how much encoded images are droppable when the blob-style API is > implemented. Maybe I'm misunderstanding this CL, but the two are different. - Currently some encoded images are not droppable because they may not exist on the disk cache. - Once the blob-style API is available, all encoded images are guaranteed to exist on the disk cache (and that's the point). What we want to measure is how much encoded images are droppable when the blob-style API is enabled, right? > > > In other words, what we want to measure is just the amount of memory of all > > encoded images. > > You mean we should measure the total amount? The blob-style API keeps alive all origin resources in the disk cache.
The CQ bit was checked by hajimehoshi@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 2016/11/17 05:54:09, haraken wrote: > On 2016/11/17 05:32:28, hajimehoshi wrote: > > On 2016/11/17 05:06:28, haraken wrote: > > > Sorry for the review delay. > > > > > > However, I don't quite understand why you want to measure this. What we want > > to > > > measure is NOT how much encoded images are droppable with the current disk > > cache > > > but how much encoded images are droppable when we implement the blob-style > API > > > on the disk cache (i.e., how much encoded images are droppable when we > > guarantee > > > all original resources are kept alive in the disk cache), right? > > > > I don't understand what's the difference between them. I intended this CL > > measures how much encoded images are droppable when the blob-style API is > > implemented. > > Maybe I'm misunderstanding this CL, but the two are different. > > - Currently some encoded images are not droppable because they may not exist on > the disk cache. > > - Once the blob-style API is available, all encoded images are guaranteed to > exist on the disk cache (and that's the point). > > What we want to measure is how much encoded images are droppable when the > blob-style API is enabled, right? IIUC not all encoded image data are dropped though it is technically possible and in my implementation an image data is droopped when MemoryCache decides to prune it. We cannot drop all image data without any conditions because this might degrades user experience (e.g. when decoding is needed, encoded image would need to be fetched but if the encoded image is in disk cache, it takes a little longer.) > > > > > > In other words, what we want to measure is just the amount of memory of all > > > encoded images. > > > > You mean we should measure the total amount? > > The blob-style API keeps alive all origin resources in the disk cache. As I described above, I don't think dropping all encoded image data is feasible.
On 2016/11/17 07:53:54, hajimehoshi wrote: > On 2016/11/17 05:54:09, haraken wrote: > > On 2016/11/17 05:32:28, hajimehoshi wrote: > > > On 2016/11/17 05:06:28, haraken wrote: > > > > Sorry for the review delay. > > > > > > > > However, I don't quite understand why you want to measure this. What we > want > > > to > > > > measure is NOT how much encoded images are droppable with the current disk > > > cache > > > > but how much encoded images are droppable when we implement the blob-style > > API > > > > on the disk cache (i.e., how much encoded images are droppable when we > > > guarantee > > > > all original resources are kept alive in the disk cache), right? > > > > > > I don't understand what's the difference between them. I intended this CL > > > measures how much encoded images are droppable when the blob-style API is > > > implemented. > > > > Maybe I'm misunderstanding this CL, but the two are different. > > > > - Currently some encoded images are not droppable because they may not exist > on > > the disk cache. > > > > - Once the blob-style API is available, all encoded images are guaranteed to > > exist on the disk cache (and that's the point). > > > > What we want to measure is how much encoded images are droppable when the > > blob-style API is enabled, right? > > IIUC not all encoded image data are dropped though it is technically possible > and in my implementation an image data is droopped when MemoryCache decides to > prune it. We cannot drop all image data without any conditions because this > might degrades user experience (e.g. when decoding is needed, encoded image > would need to be fetched but if the encoded image is in disk cache, it takes a > little longer.) > > > > > > > > > > In other words, what we want to measure is just the amount of memory of > all > > > > encoded images. > > > > > > You mean we should measure the total amount? > > > > The blob-style API keeps alive all origin resources in the disk cache. > > As I described above, I don't think dropping all encoded image data is feasible. I was thinking that what we want to measure is how much memory we can drop from a background tab by using the blob-style API, which semantically enables us to drop all encoded images. Am I misunderstanding? Also I don't think it makes sense to measure how much memory we can save by dropping encoded images that are cached in the *current* disk cache, because the blob-style API enables us to drop any encoded image semantically. How is the memory cached in the *current* disk cache related to the memory we can drop in the world with the blob-style API?
On 2016/11/17 08:08:39, haraken wrote: > On 2016/11/17 07:53:54, hajimehoshi wrote: > > On 2016/11/17 05:54:09, haraken wrote: > > > On 2016/11/17 05:32:28, hajimehoshi wrote: > > > > On 2016/11/17 05:06:28, haraken wrote: > > > > > Sorry for the review delay. > > > > > > > > > > However, I don't quite understand why you want to measure this. What we > > want > > > > to > > > > > measure is NOT how much encoded images are droppable with the current > disk > > > > cache > > > > > but how much encoded images are droppable when we implement the > blob-style > > > API > > > > > on the disk cache (i.e., how much encoded images are droppable when we > > > > guarantee > > > > > all original resources are kept alive in the disk cache), right? > > > > > > > > I don't understand what's the difference between them. I intended this CL > > > > measures how much encoded images are droppable when the blob-style API is > > > > implemented. > > > > > > Maybe I'm misunderstanding this CL, but the two are different. > > > > > > - Currently some encoded images are not droppable because they may not exist > > on > > > the disk cache. > > > > > > - Once the blob-style API is available, all encoded images are guaranteed to > > > exist on the disk cache (and that's the point). > > > > > > What we want to measure is how much encoded images are droppable when the > > > blob-style API is enabled, right? > > > > IIUC not all encoded image data are dropped though it is technically possible > > and in my implementation an image data is droopped when MemoryCache decides to > > prune it. We cannot drop all image data without any conditions because this > > might degrades user experience (e.g. when decoding is needed, encoded image > > would need to be fetched but if the encoded image is in disk cache, it takes a > > little longer.) > > > > > > > > > > > > > > In other words, what we want to measure is just the amount of memory of > > all > > > > > encoded images. > > > > > > > > You mean we should measure the total amount? > > > > > > The blob-style API keeps alive all origin resources in the disk cache. > > > > As I described above, I don't think dropping all encoded image data is > feasible. > > I was thinking that what we want to measure is how much memory we can drop from > a background tab by using the blob-style API, which semantically enables us to > drop all encoded images. Am I misunderstanding? You are right, we can drop all all encoded image. According to the current implementation of MemoryCache, onMemoryPressure calls pruneAll which prunes all resources in MemoryCache. However, to be exact, this happens only in low-end device (registering is called only on low-end devices). I'm not sure the current implementation is correct, and of course we need to move this implementation from MemoryPressure to MemoryCoordinator. > Also I don't think it makes sense to measure how much memory we can save by > dropping encoded images that are cached in the *current* disk cache, because the > blob-style API enables us to drop any encoded image semantically. How is the > memory cached in the *current* disk cache related to the memory we can drop in > the world with the blob-style API? Hmm, sorry but I don't understand what *current* mean in this context.
https://codereview.chromium.org/2499263002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/DEPS (right): https://codereview.chromium.org/2499263002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/DEPS:2: "+base/metrics/histogram_macros.h", On 2016/11/17 02:16:56, yhirano wrote: > I don't know if core can depend on base. I'm happy to defer to haraken@ on this > point. Done (Added trampoline).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/11/17 09:08:20, hajimehoshi wrote: > On 2016/11/17 08:08:39, haraken wrote: > > On 2016/11/17 07:53:54, hajimehoshi wrote: > > > On 2016/11/17 05:54:09, haraken wrote: > > > > On 2016/11/17 05:32:28, hajimehoshi wrote: > > > > > On 2016/11/17 05:06:28, haraken wrote: > > > > > > Sorry for the review delay. > > > > > > > > > > > > However, I don't quite understand why you want to measure this. What > we > > > want > > > > > to > > > > > > measure is NOT how much encoded images are droppable with the current > > disk > > > > > cache > > > > > > but how much encoded images are droppable when we implement the > > blob-style > > > > API > > > > > > on the disk cache (i.e., how much encoded images are droppable when we > > > > > guarantee > > > > > > all original resources are kept alive in the disk cache), right? > > > > > > > > > > I don't understand what's the difference between them. I intended this > CL > > > > > measures how much encoded images are droppable when the blob-style API > is > > > > > implemented. > > > > > > > > Maybe I'm misunderstanding this CL, but the two are different. > > > > > > > > - Currently some encoded images are not droppable because they may not > exist > > > on > > > > the disk cache. > > > > > > > > - Once the blob-style API is available, all encoded images are guaranteed > to > > > > exist on the disk cache (and that's the point). > > > > > > > > What we want to measure is how much encoded images are droppable when the > > > > blob-style API is enabled, right? > > > > > > IIUC not all encoded image data are dropped though it is technically > possible > > > and in my implementation an image data is droopped when MemoryCache decides > to > > > prune it. We cannot drop all image data without any conditions because this > > > might degrades user experience (e.g. when decoding is needed, encoded image > > > would need to be fetched but if the encoded image is in disk cache, it takes > a > > > little longer.) > > > > > > > > > > > > > > > > > > In other words, what we want to measure is just the amount of memory > of > > > all > > > > > > encoded images. > > > > > > > > > > You mean we should measure the total amount? > > > > > > > > The blob-style API keeps alive all origin resources in the disk cache. > > > > > > As I described above, I don't think dropping all encoded image data is > > feasible. > > > > I was thinking that what we want to measure is how much memory we can drop > from > > a background tab by using the blob-style API, which semantically enables us to > > drop all encoded images. Am I misunderstanding? > > You are right, we can drop all all encoded image. According to the current > implementation of MemoryCache, onMemoryPressure calls pruneAll which prunes all > resources in MemoryCache. > > However, to be exact, this happens only in low-end device (registering is called > only on low-end devices). I'm not sure the current implementation is correct, > and of course we need to move this implementation from MemoryPressure to > MemoryCoordinator. > > > Also I don't think it makes sense to measure how much memory we can save by > > dropping encoded images that are cached in the *current* disk cache, because > the > > blob-style API enables us to drop any encoded image semantically. How is the > > memory cached in the *current* disk cache related to the memory we can drop in > > the world with the blob-style API? > > Hmm, sorry but I don't understand what *current* mean in this context. I'm talking about the disk cache, not MemoryCache, in the first place. The blob-style API is a thing for the disk cache, not for MemoryCache. If this CL is not urgent, it might be easier to chat offline next week...
Discussed offline -- sorry, I was misunderstanding the point of this CL. LGTM.
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2499263002/#ps100001 (title: "Use platform/Histgram.h instead of adding depencency from core to base")
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": 1479706072004140,
"parent_rev": ["cc074150011f656e360ebdf2b67e48b399c25eb2", null], "commit_rev":
["38321ffc32df24644394e67f2f6747b123ab1f11", null]}
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA to estimate deroppable memory usage of encoded data size in Resources This CL adds a UMA to estimate droppable memory usage of encoded data size in Resources when reusing disk cache is implemented. If it is 100% sure that an encoded data is in disk cache, it is possible to remove the same data from memory to reduce memory usage, and the UMA measures this effectiveness. BUG=664437 ========== to ========== Add UMA to estimate deroppable memory usage of encoded data size in Resources This CL adds a UMA to estimate droppable memory usage of encoded data size in Resources when reusing disk cache is implemented. If it is 100% sure that an encoded data is in disk cache, it is possible to remove the same data from memory to reduce memory usage, and the UMA measures this effectiveness. BUG=664437 Committed: https://crrev.com/bd79bd92221f5057521d62639b7c5e84645032ef Cr-Commit-Position: refs/heads/master@{#433491} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bd79bd92221f5057521d62639b7c5e84645032ef Cr-Commit-Position: refs/heads/master@{#433491} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
