Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2140513002:
Add UMAs for SharedBuffer::lock()/unlock() (Closed)
|
Created:
4 years, 5 months ago by hiroshige Modified:
4 years, 5 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMAs for SharedBuffer::lock()/unlock()
If SharedBuffer is rarely lock()ed successfully after its unlock(), we can
remove SharedBuffer::unlock() without significant performance regressions.
To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and
unlock() counts.
This CL also fixes ResourceType enum names in histograms.xml.
ResourceType enum is introduced in 2013 and left unmodified since then, but
it is now inconsistent with C++'s Resource::Type values.
Because ResourceType enum has been unused recently (it is referenced in
histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit,
which doesn't have C++ counterpart nor UMA reports at least since 2016/01),
updating the ResourceType enum values in histograms.xml looks safe.
BUG=603791
Committed: https://crrev.com/48dcde03af18a3065e0e89b2de1e40df3e78af67
Cr-Commit-Position: refs/heads/master@{#405956}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Rebase. #Patch Set 3 : Comments #Patch Set 4 : Comments #
Total comments: 4
Patch Set 5 : Obsolete WebCore.ResourceFetcher.ResourceTypeUponCacheHit #
Total comments: 4
Patch Set 6 : Reflect comments for histograms.xml #
Messages
Total messages: 45 (27 generated)
The CQ bit was checked by hiroshige@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...
Description was changed from ========== Add UMAs for SharedBuffer::lock()/unlock() BUG=603791 ========== to ========== Add UMAs for SharedBuffer::lock()/unlock() This CL also fixes ResourceType enum names in histograms.xml. BUG=603791 ==========
hiroshige@chromium.org changed reviewers: + bashi@chromium.org, isherman@chromium.org, mkwst@chromium.org
PTAL. mkwst@ as a core/fetch OWNER isherman@ as histogram's OWNER
non-owner lgtm. Thanks for adding this! Help me understand: (total # of unlock) - (# of success lock + # of failed lock) = (# of resources never requested more than once) Does this correct? https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:567: DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, unlockHistogram, new EnumerationHistogram("Blink.SharedBuffer.Unlock", Manifest + 1)); nit: does it make sense to change the name of metric to Blink.Resource.Unlock (or move this to SharedBuffer::unlock()) ?
LGTM, but I agree that the name is a little confusing.
On 2016/07/11 08:02:27, bashi1 wrote: > non-owner lgtm. Thanks for adding this! > > Help me understand: > > (total # of unlock) - (# of success lock + # of failed lock) = (# of resources > never requested more than once) ^^^ Ah, never mind. This wouldn't be simple as such. I missed active resources. > > Does this correct? > > https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/fetch/Resource.cpp (right): > > https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/fetch/Resource.cpp:567: > DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, unlockHistogram, new > EnumerationHistogram("Blink.SharedBuffer.Unlock", Manifest + 1)); > nit: does it make sense to change the name of metric to Blink.Resource.Unlock > (or move this to SharedBuffer::unlock()) ?
> (total # of unlock) - (# of success lock + # of failed lock) I expect it is "# of resources that have its SharedBuffer unlocked but never requested/reused later (i.e. we can remove such resources from MemoryCache without loss)". https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:567: DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, unlockHistogram, new EnumerationHistogram("Blink.SharedBuffer.Unlock", Manifest + 1)); On 2016/07/11 08:02:27, bashi1 wrote: > nit: does it make sense to change the name of metric to Blink.Resource.Unlock > (or move this to SharedBuffer::unlock()) ? I also considered that option, but I names Blink.SharedBuffer.Unlock and placed the code here because: - "Blink.Resource.Unlock" looks like counting the number of Resource::unlock() calls, which is larger than that of SharedBuffer::unlock(). - This is the only callsite of SharedBuffer::unlock(), so moving this to SharedBuffer::unlock() doesn't affect the UMA result. - Putting the code in SharedBuffer::unlock() needs passing getType() to SharedBuffer, which is not nice.
https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:567: DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, unlockHistogram, new EnumerationHistogram("Blink.SharedBuffer.Unlock", Manifest + 1)); On 2016/07/11 08:15:58, hiroshige wrote: > On 2016/07/11 08:02:27, bashi1 wrote: > > nit: does it make sense to change the name of metric to Blink.Resource.Unlock > > (or move this to SharedBuffer::unlock()) ? > > I also considered that option, but I names Blink.SharedBuffer.Unlock and placed > the code here because: > - "Blink.Resource.Unlock" looks like counting the number of Resource::unlock() > calls, which is larger than that of SharedBuffer::unlock(). > - This is the only callsite of SharedBuffer::unlock(), so moving this to > SharedBuffer::unlock() doesn't affect the UMA result. > - Putting the code in SharedBuffer::unlock() needs passing getType() to > SharedBuffer, which is not nice. I see. Thanks for the explanation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:567: DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, unlockHistogram, new EnumerationHistogram("Blink.SharedBuffer.Unlock", Manifest + 1)); Please document (in the header file, where the enum is defined) that this enum is now being used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:992: DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, failedLockHistogram, new EnumerationHistogram("Blink.SharedBuffer.FailedLock", Manifest + 1)); Using "Manifest + 1" as a boundary value is a bit fragile: If anyone updates the enum, they might not notice that they should update this callsite as well. For this reason, UMA enums typically have a "COUNT" or "LAST" meta-entry, which is much more likely to remain correctly up-to-date. https://codereview.chromium.org/2140513002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2140513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3837: + https://crbug.com/603791 It's not clear to me, from looking at the bug, what the motivation for recording these metrics is. Could you please clarify? https://codereview.chromium.org/2140513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:88316: + <int value="13" label="Manifest"/> This is not a safe change -- the result is that bucket 11 will be labeled as "ImportResource", but will in fact be a mix of data coming from "Shader" and "ImportResource" (and bucket 12 will have a similar problem). Do you know how long ago the C++ enum was changed out from under the affected histogram(s)? I'd recommend renaming any affected histogram(s) so that the data is clearly interpretable. (Note that there will always be a long tail of users who are "stuck" on older versions of Chrome, and who will continue to upload metrics. So even if the enum change happened a year ago, the data will continue to be influenced by users who are uploading data using the older enum semantics.)
Description was changed from ========== Add UMAs for SharedBuffer::lock()/unlock() This CL also fixes ResourceType enum names in histograms.xml. BUG=603791 ========== to ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. BUG=603791 ==========
Description was changed from ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. BUG=603791 ========== to ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. ResourceType enum is introduced in 2013 and left unmodified since then, but it is now inconsistent with C++'s Resource::Type values. Because ResourceType enum has been unused recently (it is referenced in histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit, which doesn't have C++ counterpart nor UMA reports in recent 28 days), updating the ResourceType enum values in histograms.xml looks safe. BUG=603791 ==========
The CQ bit was checked by hiroshige@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 hiroshige@chromium.org to run a CQ dry run
https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:567: DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, unlockHistogram, new EnumerationHistogram("Blink.SharedBuffer.Unlock", Manifest + 1)); On 2016/07/11 19:28:33, Ilya Sherman wrote: > Please document (in the header file, where the enum is defined) that this enum > is now being used to back an UMA histogram, and should therefore be treated as > append-only. Done. https://codereview.chromium.org/2140513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:992: DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, failedLockHistogram, new EnumerationHistogram("Blink.SharedBuffer.FailedLock", Manifest + 1)); On 2016/07/11 19:28:33, Ilya Sherman wrote: > Using "Manifest + 1" as a boundary value is a bit fragile: If anyone updates the > enum, they might not notice that they should update this callsite as well. For > this reason, UMA enums typically have a "COUNT" or "LAST" meta-entry, which is > much more likely to remain correctly up-to-date. Added |kLastResourceType|. I avoid adding |Last| entry directly to |enum Type|, because adding |Last| causes compile errors like: error: enumeration value 'Last' not handled in switch ... https://codereview.chromium.org/2140513002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2140513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3837: + https://crbug.com/603791 On 2016/07/11 19:28:33, Ilya Sherman wrote: > It's not clear to me, from looking at the bug, what the motivation for recording > these metrics is. Could you please clarify? Updated the CL description. https://codereview.chromium.org/2140513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:88316: + <int value="13" label="Manifest"/> On 2016/07/11 19:28:33, Ilya Sherman wrote: > This is not a safe change -- the result is that bucket 11 will be labeled as > "ImportResource", but will in fact be a mix of data coming from "Shader" and > "ImportResource" (and bucket 12 will have a similar problem). Do you know how > long ago the C++ enum was changed out from under the affected histogram(s)? I'd > recommend renaming any affected histogram(s) so that the data is clearly > interpretable. (Note that there will always be a long tail of users who are > "stuck" on older versions of Chrome, and who will continue to upload metrics. > So even if the enum change happened a year ago, the data will continue to be > influenced by users who are uploading data using the older enum semantics.) This enum is used by WebCore.ResourceFetcher.ResourceTypeUponCacheHit, but it seems no longer used: - No C++ code touching ResourceTypeUponCacheHit, and - No UMA count in recent 28 days. Probably WebCore.ResourceFetcher.ResourceTypeUponCacheHit is removed quite long time ago and I expect it's safe to modify the values here (and deprecate WebCore.ResourceFetcher.ResourceTypeUponCacheHit). +clamy@ as the owner of the metric.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hiroshige@chromium.org changed reviewers: + clamy@chromium.org
+clamy@ for https://codereview.chromium.org/2140513002/diff/1/tools/metrics/histograms/hi... Since when WebCore.ResourceFetcher.ResourceTypeUponCacheHit has been unused?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for updating the CL description. Could you please also update the bug, since that is where the histogram description text points to? https://codereview.chromium.org/2140513002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2140513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.h:61: // existing |Type|. When adding a new |Type|, append it at the last and nit: s/at the last/at the end https://codereview.chromium.org/2140513002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2140513002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:88378: + <int value="13" label="Manifest"/> If the only existing histogram that used this enum is no longer recorded, then this change is mostly safe. In that case, though, please mark that histogram as <obsolete>, and add a note indicating that the enum labels for it are not entirely reliable.
Description was changed from ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. ResourceType enum is introduced in 2013 and left unmodified since then, but it is now inconsistent with C++'s Resource::Type values. Because ResourceType enum has been unused recently (it is referenced in histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit, which doesn't have C++ counterpart nor UMA reports in recent 28 days), updating the ResourceType enum values in histograms.xml looks safe. BUG=603791 ========== to ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. ResourceType enum is introduced in 2013 and left unmodified since then, but it is now inconsistent with C++'s Resource::Type values. Because ResourceType enum has been unused recently (it is referenced in histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit, which doesn't have C++ counterpart nor UMA reports at least since 2016/01), updating the ResourceType enum values in histograms.xml looks safe. BUG=603791 ==========
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by hiroshige@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/2140513002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2140513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.h:61: // existing |Type|. When adding a new |Type|, append it at the last and On 2016/07/12 21:23:17, Ilya Sherman wrote: > nit: s/at the last/at the end Done. https://codereview.chromium.org/2140513002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2140513002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:88378: + <int value="13" label="Manifest"/> On 2016/07/12 21:23:17, Ilya Sherman wrote: > If the only existing histogram that used this enum is no longer recorded, then > this change is mostly safe. In that case, though, please mark that histogram as > <obsolete>, and add a note indicating that the enum labels for it are not > entirely reliable. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Could you please also update the bug, > since that is where the histogram description text points to? Added. https://bugs.chromium.org/p/chromium/issues/detail?id=603791#c4
LGTM % a final couple of nits. Thanks! https://codereview.chromium.org/2140513002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2140513002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62982: + been changed over time. nit: s/This UMA/This metric https://codereview.chromium.org/2140513002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62982: + been changed over time. nit: s/ResourceType enums/the displayed enum labels
https://codereview.chromium.org/2140513002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2140513002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62982: + been changed over time. On 2016/07/15 21:07:27, Ilya Sherman wrote: > nit: s/This UMA/This metric Done.
https://codereview.chromium.org/2140513002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2140513002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62982: + been changed over time. On 2016/07/15 21:07:27, Ilya Sherman wrote: > nit: s/ResourceType enums/the displayed enum labels Done.
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, bashi@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2140513002/#ps160001 (title: "Reflect comments for histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. ResourceType enum is introduced in 2013 and left unmodified since then, but it is now inconsistent with C++'s Resource::Type values. Because ResourceType enum has been unused recently (it is referenced in histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit, which doesn't have C++ counterpart nor UMA reports at least since 2016/01), updating the ResourceType enum values in histograms.xml looks safe. BUG=603791 ========== to ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. ResourceType enum is introduced in 2013 and left unmodified since then, but it is now inconsistent with C++'s Resource::Type values. Because ResourceType enum has been unused recently (it is referenced in histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit, which doesn't have C++ counterpart nor UMA reports at least since 2016/01), updating the ResourceType enum values in histograms.xml looks safe. BUG=603791 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. ResourceType enum is introduced in 2013 and left unmodified since then, but it is now inconsistent with C++'s Resource::Type values. Because ResourceType enum has been unused recently (it is referenced in histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit, which doesn't have C++ counterpart nor UMA reports at least since 2016/01), updating the ResourceType enum values in histograms.xml looks safe. BUG=603791 ========== to ========== Add UMAs for SharedBuffer::lock()/unlock() If SharedBuffer is rarely lock()ed successfully after its unlock(), we can remove SharedBuffer::unlock() without significant performance regressions. To evaluate this assumption, this CL adds UMAs for SharedBuffer::lock() and unlock() counts. This CL also fixes ResourceType enum names in histograms.xml. ResourceType enum is introduced in 2013 and left unmodified since then, but it is now inconsistent with C++'s Resource::Type values. Because ResourceType enum has been unused recently (it is referenced in histograms.xml only from WebCore.ResourceFetcher.ResourceTypeUponCacheHit, which doesn't have C++ counterpart nor UMA reports at least since 2016/01), updating the ResourceType enum values in histograms.xml looks safe. BUG=603791 Committed: https://crrev.com/48dcde03af18a3065e0e89b2de1e40df3e78af67 Cr-Commit-Position: refs/heads/master@{#405956} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/48dcde03af18a3065e0e89b2de1e40df3e78af67 Cr-Commit-Position: refs/heads/master@{#405956} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
