Memory usage increase caused by storing glyph bounds in GlyphMetricsMap http://trac.webkit.org/changeset/57215 I'm working on a fix for this
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.
<rdar://problem/7855777>
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!
Attachment 54665 [details] committed as <http://trac.webkit.org/projects/webkit/changeset/58467>.
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>.