Summary: | REGRESSION: Memory usage increase caused by storing glyph bounds in GlyphMetricsMap | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adele Peterson <adele> | ||||||||||||
Component: | Text | Assignee: | Adele Peterson <adele> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, eric, mitz, ojan, slewis, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Adele Peterson
2010-04-21 11:25:32 PDT
Created attachment 53975 [details]
preliminary patch
I'm still testing the memory benefit here, but I'd like to get preliminary review while I'm confirming there's some memory usage benefit to this change.
Comment on attachment 53975 [details] preliminary patch > + Break GlyphMetricsMap into GlyphBoundsMap and GlyphWidthMap. One could make the case for making a template GlyphMetricsMap<> and having GlyphBoundsMap and GlyphWidthMap be instances of that template. > + * platform/graphics/GlyphWidthMap.cpp: Copied from platform/graphics/GlyphMetricsMap.cpp. Perhaps a more elegant way to represent this part of the change would be to resurrect the old GlyphWidthMap.{cpp,h} using SVN magic. > + * platform/graphics/SimpleFontData.h: (WebCore::SimpleFontData::metricsForGlyph): > + This method is only used by ComplexTextController, so we'll keep it, and add the GlyphMetrics struct > + in this header file. I think ComplexTextController should just fetch the width and the bounds separately. There’s little value in introducing the struct here (or anywhere). > -GlyphMetricsMap::GlyphMetricsPage* GlyphMetricsMap::locatePageSlowCase(unsigned pageNumber) > + Extra whitespace. > + for (unsigned i = 0; i < GlyphBoundsPage::size; i++) > + page->setBoundsForIndex(i, unknownBounds); > + And here. > Index: WebCore/platform/graphics/GlyphBoundsMap.h > =================================================================== > --- WebCore/platform/graphics/GlyphBoundsMap.h (revision 57946) > +++ WebCore/platform/graphics/GlyphBoundsMap.h (working copy) > + > + typedef unsigned short Glyph; Does this not conflict with the same typedef in GlyphWidthMap.h? Maybe it’s okay because they’re the same? > + const float cGlyphSizeUnknown = -1; And this? > Index: WebCore/platform/graphics/GlyphWidthMap.cpp > =================================================================== > --- WebCore/platform/graphics/GlyphWidthMap.cpp (revision 57946) > +++ WebCore/platform/graphics/GlyphWidthMap.cpp (working copy) > namespace WebCore { > + GlyphWidthMap::GlyphWidthPage* GlyphWidthMap::locatePageSlowCase(unsigned pageNumber) We don’t indent for namespace in .cpp files. > #include "FontPlatformData.h" > -#include "GlyphMetricsMap.h" > +#include "GlyphBoundsMap.h" > +#include "GlyphWidthMap.h" > #include "GlyphPageTreeNode.h" These are no longer sorted. > +struct GlyphMetrics { > + float horizontalAdvance; > + FloatRect boundingBox; > +}; > + Extra whitespace there. Like I said, I don’t think this is needed. > enum GlyphMetricsMode { GlyphBoundingBox, GlyphWidthOnly }; Or this. > float widthForGlyph(Glyph glyph) const { return metricsForGlyph(glyph, GlyphWidthOnly).horizontalAdvance; } > GlyphMetrics metricsForGlyph(Glyph, GlyphMetricsMode = GlyphBoundingBox) const; Just replace this with FloatRect boundsForGlyph(…) const; Comment on attachment 53975 [details]
preliminary patch
OK will make changes and post a new patch. Thanks, Dan!
This is trying to fix a memory regression on the Membuster test. Google's page_cycler saw a similar regression from the same change. See http://bugs.webkit.org/show_bug.cgi?id=37292 Created attachment 54120 [details]
patch
still need to test on Windows...and confirm memory win...
Comment on attachment 54120 [details]
patch
update for windows coming soon...
Created attachment 54122 [details]
patch
and one more time...
Comment on attachment 54122 [details]
patch
r=me pending perf measurements
This looks to have been an ~10 MB improvement on the regression, but I believe there is still some regression left. Committed revision 58192. Leaving this open until we have more test results. Created attachment 54392 [details]
Patch to further reduce memory use
Comment on attachment 54392 [details]
Patch to further reduce memory use
That's awesome! It's probably safe to assume there aren't any fonts that stupidly have a non-zero width zeroWidthSpace? What about ahem? I doubt we care for any real-world font though.
(In reply to comment #13) > (From update of attachment 54392 [details]) > It's probably safe to assume there aren't any fonts that > stupidly have a non-zero width zeroWidthSpace? I don’t think that’s a safe assumption, which is why the code to force ZWS to have zero width was added in the first place (by me, if I remember correctly). Created attachment 54665 [details]
Don’t store the width and bounds of the ZERO WIDTH SPACE glyph in the metrics maps
Comment on attachment 54665 [details] Don’t store the width and bounds of the ZERO WIDTH SPACE glyph in the metrics maps > + (WebCore::SimpleFontData::platformGlyphInit): Set m_zeroWidthSpaceGlyph. Donât creat entries typos! http://trac.webkit.org/changeset/58467 might have broken Leopard Intel Release (Tests) (In reply to comment #18) > http://trac.webkit.org/changeset/58467 might have broken Leopard Intel Release > (Tests) Which I tried to fix in <http://trac.webkit.org/projects/webkit/changeset/58474>. |