Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088183002:
Add UMA for hb_face_t instantiation type (Closed)
|
Created:
4 years, 6 months ago by drott Modified:
4 years, 5 months ago CC:
ajuma+watch_chromium.org, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for hb_face_t instantiation type
Counting on non-Mac platforms what kind of hb_face_t instantiation
method is used and whether the in-place access was successful. The goal
is to identify whether we need the fallback codepath at all.
BUG=621878
Committed: https://crrev.com/85934478433c6484b352e348f149e5e256be04d3
Cr-Commit-Position: refs/heads/master@{#402779}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Mention that counted on non-mac only #Patch Set 4 : Rebased #
Total comments: 4
Patch Set 5 : Move from own enumeration to BooleanSuccess #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== Add UMA for hb_face_t instantiation type Counting on non-Mac platforms what kind of hb_face_t instantiation method is used and whether the in-place access was successful. The goal is to identify whether we need the fallback codepath at all. BUG=621878 Zero-copy font table access for HarfBuzz Our previous implementation relied on the copy function harfBuzzSkiaGetTable() to create a font table copy, which is held by hb_face_t and hb_font_t in turn. This allocation is additional overhead in terms of memory consumption and allocation cost. Using Skia's openStream() API we can retrieve the SkStreamAsset for the SkTypeface. On desktop Linux, on Android and on Windows my experimentation shows that this SkStreamAsset is always accessible in memory, since the getMemoryBase() returns a valid address. The openStream() call itself duplicates and SkStream internally, but does not duplicate the underlying in-memory data. From The SkStreamAsset we can wrap the font data into an hb_blob_t for consumption by hb_face_create. openStream() transfers ownerships of the returned SkStreaAsset to the caller, so we have to specifiy a destruction function to HarfBuzz when hb_face_t releases the hb_blob_t. This CL saves all time and space for additional table allocations required for text shaping, which can range from a few kilobytes for a set of Latin fonts to hundreds of kilobytes for multiple fonts with large tables accessed by HarfBuzz. BUG=621878 ========== to ========== Add UMA for hb_face_t instantiation type Counting on non-Mac platforms what kind of hb_face_t instantiation method is used and whether the in-place access was successful. The goal is to identify whether we need the fallback codepath at all. BUG=621878 ==========
Mention that counted on non-mac only
The CQ bit was checked by drott@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Rebased
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 drott@chromium.org
drott@chromium.org changed reviewers: + behdad@chromium.org, eae@chromium.org, holte@chromium.org, jwd@chromium.org, mpearson@chromium.org
The CQ bit was checked by drott@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/2088183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2088183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:60: enum HarfBuzzFaceInstantiationTypeUMA { Please put the standard warning here that these values are written to logs and should not be renumbered or deleted. Or, why is this an enumerated histogram rather than simply a BooleanHistogram? Do you expect to expand it later? https://codereview.chromium.org/2088183002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2088183002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3566: +<histogram name="Blink.Fonts.HarfBuzzFaceInstantiationType"> please use the enum= construct. You may also want to revise the description once you do so.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Move from own enumeration to BooleanSuccess
Thanks for the review, CL updated addressing your comments. https://codereview.chromium.org/2088183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2088183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:60: enum HarfBuzzFaceInstantiationTypeUMA { On 2016/06/27 at 21:28:41, Mark P wrote: > Please put the standard warning here that these values are written to logs and should not be renumbered or deleted. > > Or, why is this an enumerated histogram rather than simply a BooleanHistogram? Do you expect to expand it later? I was initially considering reporting the different instantiation type on Mac, but that's unrelated, so I am switching to BooleanSuccess. https://codereview.chromium.org/2088183002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2088183002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3566: +<histogram name="Blink.Fonts.HarfBuzzFaceInstantiationType"> On 2016/06/27 at 21:28:41, Mark P wrote: > please use the enum= construct. You may also want to revise the description once you do so. Done, changed to enum="BooleanSuccess".
lgtm
The CQ bit was checked by drott@chromium.org
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA for hb_face_t instantiation type Counting on non-Mac platforms what kind of hb_face_t instantiation method is used and whether the in-place access was successful. The goal is to identify whether we need the fallback codepath at all. BUG=621878 ========== to ========== Add UMA for hb_face_t instantiation type Counting on non-Mac platforms what kind of hb_face_t instantiation method is used and whether the in-place access was successful. The goal is to identify whether we need the fallback codepath at all. BUG=621878 Committed: https://crrev.com/85934478433c6484b352e348f149e5e256be04d3 Cr-Commit-Position: refs/heads/master@{#402779} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/85934478433c6484b352e348f149e5e256be04d3 Cr-Commit-Position: refs/heads/master@{#402779}
Message was sent while issue was closed.
LGTM post-facto |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
