Bug 37936 – REGRESSION: Memory usage increase caused by storing glyph bounds in GlyphMetricsMap

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Adele Peterson]

Memory usage increase caused by storing glyph bounds in GlyphMetricsMap

http://trac.webkit.org/changeset/57215

I'm working on a fix for this

[Adele Peterson]

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.

[Adele Peterson]

<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;

[Adele Peterson]

Comment on attachment 53975 [details]
preliminary patch

OK will make changes and post a new patch.  Thanks, Dan!

[Stephanie Lewis]

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

[Adele Peterson]

Created attachment 54120 [details]
patch

still need to test on Windows...and confirm memory win...

[Adele Peterson]

Comment on attachment 54120 [details]
patch

update for windows coming soon...

[Adele Peterson]

Created attachment 54122 [details]
patch

and one more time...

Comment on attachment 54122 [details]
patch

r=me pending perf measurements

[Stephanie Lewis]

This looks to have been an ~10 MB improvement on the regression, but I believe there is still some regression left.

[Adele Peterson]

Committed revision 58192.  Leaving this open until we have more test results.

Created attachment 54392 [details]
Patch to further reduce memory use

[Ojan Vafai]

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

[Adele Peterson]

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>.

[WebKit Review Bot]

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>.