Bug 37936 - REGRESSION: Memory usage increase caused by storing glyph bounds in GlyphMetricsMap
Summary: REGRESSION: Memory usage increase caused by storing glyph bounds in GlyphMetr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Adele Peterson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-04-21 11:25 PDT by Adele Peterson
Modified: 2010-04-28 21:24 PDT (History)
6 users (show)

See Also:


Attachments
preliminary patch (62.00 KB, patch)
2010-04-21 11:38 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (39.23 KB, patch)
2010-04-22 19:09 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (40.45 KB, patch)
2010-04-22 19:47 PDT, Adele Peterson
mitz: review+
Details | Formatted Diff | Diff
Patch to further reduce memory use (3.12 KB, patch)
2010-04-27 00:13 PDT, mitz
no flags Details | Formatted Diff | Diff
Don’t store the width and bounds of the ZERO WIDTH SPACE glyph in the metrics maps (4.57 KB, patch)
2010-04-28 20:04 PDT, mitz
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2010-04-21 11:25:32 PDT
Memory usage increase caused by storing glyph bounds in GlyphMetricsMap

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

I'm working on a fix for this
Comment 1 Adele Peterson 2010-04-21 11:38:17 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 2 Adele Peterson 2010-04-21 11:39:48 PDT
<rdar://problem/7855777>
Comment 3 mitz 2010-04-21 11:56:16 PDT
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 4 Adele Peterson 2010-04-21 14:10:53 PDT
Comment on attachment 53975 [details]
preliminary patch

OK will make changes and post a new patch.  Thanks, Dan!
Comment 5 Stephanie Lewis 2010-04-21 16:12:07 PDT
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
Comment 6 Adele Peterson 2010-04-22 19:09:27 PDT
Created attachment 54120 [details]
patch

still need to test on Windows...and confirm memory win...
Comment 7 Adele Peterson 2010-04-22 19:22:20 PDT
Comment on attachment 54120 [details]
patch

update for windows coming soon...
Comment 8 Adele Peterson 2010-04-22 19:47:07 PDT
Created attachment 54122 [details]
patch

and one more time...
Comment 9 mitz 2010-04-22 19:59:38 PDT
Comment on attachment 54122 [details]
patch

r=me pending perf measurements
Comment 10 Stephanie Lewis 2010-04-23 15:04:49 PDT
This looks to have been an ~10 MB improvement on the regression, but I believe there is still some regression left.
Comment 11 Adele Peterson 2010-04-23 15:17:30 PDT
Committed revision 58192.  Leaving this open until we have more test results.
Comment 12 mitz 2010-04-27 00:13:37 PDT
Created attachment 54392 [details]
Patch to further reduce memory use
Comment 13 Ojan Vafai 2010-04-27 07:31:21 PDT
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.
Comment 14 mitz 2010-04-28 19:58:00 PDT
(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).
Comment 15 mitz 2010-04-28 20:04:14 PDT
Created attachment 54665 [details]
Don’t store the width and bounds of the ZERO WIDTH SPACE glyph in the metrics maps
Comment 16 Adele Peterson 2010-04-28 20:06:43 PDT
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!
Comment 18 WebKit Review Bot 2010-04-28 20:55:41 PDT
http://trac.webkit.org/changeset/58467 might have broken Leopard Intel Release (Tests)
Comment 19 mitz 2010-04-28 21:24:26 PDT
(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>.