Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1346993006:
Strip font name of special characters before dumping (Closed)
DescriptionStrip font name of special characters before dumping
The font name can contain special characters in some OS that is
considered as delimiters ('/') by the traces. This causes error in the
dump name. This CL strips the fone name obtained before adding it to
traces.
This also adds discardable memory size for resources in cache.
BUG=532838
Committed: https://skia.googlesource.com/skia/+/60df542afc3b792c6d563db2dfe233f65a2c3632
Patch Set 1 #
Total comments: 10
Patch Set 2 : Use std::isalnum #Patch Set 3 : Nits. #
Total comments: 4
Patch Set 4 : Adding a comment suggested. #Patch Set 5 : Nit. #Messages
Total messages: 23 (8 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
Do you think this is good enough or should i do this processing in chrome side? I am doing it here since the name can contain '/'.
LGTM with some comments https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:8: #include <ctype.h> you don't need ctype, just <string> https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:437: SkString strippedName; you can make this more efficient giving a capacity hint using the original string name, i.e. strippedName(fontName.szie()) https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:438: for (int index = 0; index < fontName.size(); ++index) { nit: size_t https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:439: if (isalnum(fontName[index])) { std::isalnum https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:441: } else: += '_'
Thanks made changes. https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:8: #include <ctype.h> On 2015/09/22 17:30:35, Primiano Tucci wrote: > you don't need ctype, just <string> Done. https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:437: SkString strippedName; On 2015/09/22 17:30:35, Primiano Tucci wrote: > you can make this more efficient giving a capacity hint using the original > string name, i.e. > strippedName(fontName.szie()) Done. https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:438: for (int index = 0; index < fontName.size(); ++index) { On 2015/09/22 17:30:35, Primiano Tucci wrote: > nit: size_t Done. https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:439: if (isalnum(fontName[index])) { On 2015/09/22 17:30:35, Primiano Tucci wrote: > std::isalnum Done. https://codereview.chromium.org/1346993006/diff/1/src/core/SkGlyphCache.cpp#n... src/core/SkGlyphCache.cpp:441: } On 2015/09/22 17:30:35, Primiano Tucci wrote: > else: += '_' Done.
ssid@chromium.org changed reviewers: + reed@google.com
+reed This adds the change from previous CL for the resouce cache and fix for special characters in font names. I am not sure if I can include string in skia. PTAL, Thanks.
https://codereview.chromium.org/1346993006/diff/40001/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/1346993006/diff/40001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:437: for (size_t index = 0; index < fontName.size(); ++index) { What is the formal requirement for strings in your API? This above "clean-up" looks odd to have to do it here. Can this not be done canonically for all strings at the receiving end? https://codereview.chromium.org/1346993006/diff/40001/src/core/SkResourceCach... File src/core/SkResourceCache.cpp (right): https://codereview.chromium.org/1346993006/diff/40001/src/core/SkResourceCach... src/core/SkResourceCache.cpp:682: dump->dumpNumericValue(dumpName.c_str(), "discardable_size", "bytes", rec.bytesUsed()); Why is this needed? Can we not determine the size of the discardable allocation object itself? The bytesUsed() method I've written combines all of the allocations associated with the record, both discardable and malloc (typically the malloc part is quite small).
https://codereview.chromium.org/1346993006/diff/40001/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/1346993006/diff/40001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:437: for (size_t index = 0; index < fontName.size(); ++index) { On 2015/09/23 13:21:29, reed1 wrote: > What is the formal requirement for strings in your API? This above "clean-up" > looks odd to have to do it here. Can this not be done canonically for all > strings at the receiving end? Ah yes. So the dump name is required to be seperated by a '/' to make a tree structure. As you can see the dump name is given as : :sk_glyph_cac he/font_name_1/index" This means that glyph cache has fonts and each type of font has many indices. Here '/' is a special seperator for the dump. But if the font name has this character then it would change the tree brhavior. So to avoid that this sanitize is added. https://codereview.chromium.org/1346993006/diff/40001/src/core/SkResourceCach... File src/core/SkResourceCache.cpp (right): https://codereview.chromium.org/1346993006/diff/40001/src/core/SkResourceCach... src/core/SkResourceCache.cpp:682: dump->dumpNumericValue(dumpName.c_str(), "discardable_size", "bytes", rec.bytesUsed()); On 2015/09/23 13:21:29, reed1 wrote: > Why is this needed? Can we not determine the size of the discardable allocation > object itself? The bytesUsed() method I've written combines all of the > allocations associated with the record, both discardable and malloc (typically > the malloc part is quite small). The system works like this: Skia tells discardable memory interface to dump the particular object allocated -> Discardable memory dumps the size of the object only if the object is still alive in discardable manager. If the object has been purged by the discardable manager, then it records 0 for this object. Now skia has no way to tell how much memory it potentially could have allocated and wants to keep around if possible. This discardable_size is an extra attribute to tell that skia cache would keep around so much of memory if there was infinite memory available in the system. Currently the size of the cache shown is coorelated with the discardable manager and only displays memory which is alive.
> https://codereview.chromium.org/1346993006/diff/40001/src/core/SkResourceCach... > src/core/SkResourceCache.cpp:682: dump->dumpNumericValue(dumpName.c_str(), > "discardable_size", "bytes", rec.bytesUsed()); > On 2015/09/23 13:21:29, reed1 wrote: > > Why is this needed? Can we not determine the size of the discardable > allocation > > object itself? The bytesUsed() method I've written combines all of the > > allocations associated with the record, both discardable and malloc (typically > > the malloc part is quite small). > > The system works like this: > Skia tells discardable memory interface to dump the particular object allocated > -> Discardable memory dumps the size of the object only if the object is still > alive in discardable manager. If the object has been purged by the discardable > manager, then it records 0 for this object. > > Now skia has no way to tell how much memory it potentially could have allocated > and wants to keep around if possible. This discardable_size is an extra > attribute to tell that skia cache would keep around so much of memory if there > was infinite memory available in the system. > > Currently the size of the cache shown is coorelated with the discardable manager > and only displays memory which is alive. Please see the mail I have sent you recently with a trace screenshot of skia. This shows how the discardable_size varies from actual size.
lgtm
Perhaps this distinction between "live discardable" and "dead discardable" should be documented in the code itself, so that a future maintainer will know why we are reporting each.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1346993006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1346993006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1346993006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1346993006/80001
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 ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1346993006/#ps80001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1346993006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1346993006/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/60df542afc3b792c6d563db2dfe233f65a2c3632 |
