Added purge method to SimpleFontData and PlatformFontData. (issue 2178883002 by tasak@google.com)

[ta...@google.com]

Reviewers: haraken, esprehn, bashi1
CL: https://codereview.chromium.org/2178883002/

Message:
Would you review this CL?


Description:
Added purge method to SimpleFontData and PlatformFontData.

Purge the followings:
- m_harfBuzzFace and m_typeface in PlatformFontData, and
- m_glyphToBoundsMap in SimpleFontData

BUG=

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+16, -0 lines):
M third_party/WebKit/Source/platform/fonts/FontPlatformData.h
M third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
M third_party/WebKit/Source/platform/fonts/SimpleFontData.h
M third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp


Index: third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
diff --git a/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp b/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
index e6f42cb4a1f9505f00dca43dee4d939932aa7e01..b2014bda4d0931439517c70062440b3eba60489c 100644
--- a/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
+++ b/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
@@ -346,4 +346,10 @@ PassRefPtr<SharedBuffer> FontPlatformData::openTypeTable(SkFontTableTag tag) con
return buffer.release();
}

+void FontPlatformData::purgeMemory()
+{
+ m_typeface = nullptr;
+ m_harfBuzzFace = nullptr;
+}
+
} // namespace blink
Index: third_party/WebKit/Source/platform/fonts/FontPlatformData.h
diff --git a/third_party/WebKit/Source/platform/fonts/FontPlatformData.h b/third_party/WebKit/Source/platform/fonts/FontPlatformData.h
index faf4636cfcb6d23a6c4163f88897ff05e21e36bd..d4218d71200b839790598ebb3125d6891b8c9924 100644
--- a/third_party/WebKit/Source/platform/fonts/FontPlatformData.h
+++ b/third_party/WebKit/Source/platform/fonts/FontPlatformData.h
@@ -136,6 +136,8 @@ public:
int paintTextFlags() const { return m_paintTextFlags; }
#endif

+ void purgeMemory();
+
private:
#if OS(WIN)
void querySystemForRenderStyle();
Index: third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp
diff --git a/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp b/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp
index efed5b9083f72d60fc2b920d60074b69e495437f..ddf9765e553ae6b24eec02d8cc62323adb84659c 100644
--- a/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp
+++ b/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp
@@ -447,4 +447,10 @@ bool SimpleFontData::fillGlyphPage(GlyphPage* pageToFill, unsigned offset, unsig
return haveGlyphs;
}

+void SimpleFontData::purgeMemory()
+{
+ m_glyphToBoundsMap.reset(nullptr);
+ m_platformData.purgeMemory();
+}
+
} // namespace blink
Index: third_party/WebKit/Source/platform/fonts/SimpleFontData.h
diff --git a/third_party/WebKit/Source/platform/fonts/SimpleFontData.h b/third_party/WebKit/Source/platform/fonts/SimpleFontData.h
index 9cbeb8b371988ab6d39cf1ab155293a2773d26e4..82a4473068cb85e7de28ff8ffc777d74b735eeec 100644
--- a/third_party/WebKit/Source/platform/fonts/SimpleFontData.h
+++ b/third_party/WebKit/Source/platform/fonts/SimpleFontData.h
@@ -127,6 +127,8 @@ public:
// Implemented by the platform.
virtual bool fillGlyphPage(GlyphPage* pageToFill, unsigned offset, unsigned length, UChar* buffer, unsigned bufferLength) const;

+ void purgeMemory();
+
protected:
SimpleFontData(const FontPlatformData&, PassRefPtr<CustomFontData> customData, bool isTextOrientationFallback = false);

[har...@chromium.org]

Who calls the purgeMemory?

Is it a part of landing https://codereview.chromium.org/2130683002/?

https://codereview.chromium.org/2178883002/

[ta...@google.com]

On 2016/07/25 09:21:38, haraken wrote:
> Who calls the purgeMemory?

Blink's MemoryCoordinator will call.

> Is it a part of landing https://codereview.chromium.org/2130683002/

Yes.



https://codereview.chromium.org/2178883002/

[har...@chromium.org]

On 2016/07/25 09:28:31, tasak wrote:
> On 2016/07/25 09:21:38, haraken wrote:
> > Who calls the purgeMemory?
>
> Blink's MemoryCoordinator will call.
>
> > Is it a part of landing https://codereview.chromium.org/2130683002/
>
> Yes.

I'd suggest reaching consensus on the purge + suspend before starting landing
the CLs. The purgeMemory() will touch a broad area of the code base, so it's
better to send Intent-to-Implement before making the changes. What do you think?



https://codereview.chromium.org/2178883002/

[ta...@google.com]

I was requested by bashi-san. I don't want to change current MemoryCoordinator
to invoke this method.
Instead, the future? MemoryCoordinator will invoke.


https://codereview.chromium.org/2178883002/

[har...@chromium.org]

I'm fine with landing the changes without making changes to the current MC
(which totally makes sense), but we'll need to reach consensus (i.e., send
Intent-to-Implement) before landing things that affect a broad area of the code
base.

Maybe should we chat about the plan via VC tomorrow?


https://codereview.chromium.org/2178883002/

[ta...@google.com]

So do you mean purge-and-suspend consensus?

I would like to add this change not for purge-and-suspend. Instead, for the
future MemoryCoordinator (purge / throttle).


https://codereview.chromium.org/2178883002/

[dr...@chromium.org]

Drive by: This looks a bit coarse to me. I am all for memory improvements in the
area of fonts but it makes sense to look at this in a bit more detail. (See some
of my previous CLs, calling FontCache::invalidate on low memory, zero copy font
blob access.)

We should find a way to contain unbounded growth of the glyphToBoundsMap, or try
to remove it by accessing the Skia-cached information faster.

On a larger scale: It makes sense to have better font lifecycle management and
somehow create a link between Oilpan and the fonts that are actually in use in
order to avoid unbounded growth.


https://codereview.chromium.org/2178883002/diff/1/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
File third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
(right):

https://codereview.chromium.org/2178883002/diff/1/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp#newcode351
third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp:351:
m_typeface = nullptr;
I don't think this is safe at the moment.

https://codereview.chromium.org/2178883002/

[ba...@chromium.org]

I was thinking what I can do without worrying about other tasks (e.g. who calls
purge when) and I thought we'll implement purge anyway. I was about to start
writing purge in blink but tasak-san is already doing the same so I asked him to
do that. It totally makes sense to send intent-to-implement first though. If you
don't want to do this at this point I'll seek another task. Tasak-san, sorry for
bothering.

https://codereview.chromium.org/2178883002/

[ta...@google.com]

On 2016/07/25 12:28:07, drott wrote:
> Drive by: This looks a bit coarse to me. I am all for memory improvements in
the
> area of fonts but it makes sense to look at this in a bit more detail. (See
some
> of my previous CLs, calling FontCache::invalidate on low memory, zero copy
font
> blob access.)

> We should find a way to contain unbounded growth of the glyphToBoundsMap, or
try
> to remove it by accessing the Skia-cached information faster.

I think, the way to contain unbounded growth is required for throttle of
MemoryCoordinator.
This is for MemoryCoordinator's purge. I think, this is different.

> On a larger scale: It makes sense to have better font lifecycle management and
> somehow create a link between Oilpan and the fonts that are actually in use in
> order to avoid unbounded growth.

I would like to ask what's "actually in use". If a renderer is invisible
(background), the fonts are not used. So we cannot purge? Or if critical memory
notification is invoked, we cannot release such caches?
I think, the patch is not for lifecycle management. I agree that the change
affects CPU performance. But if we have no memory, we should keep such cache?


https://codereview.chromium.org/2178883002/

[dr...@chromium.org]

On 2016/07/26 at 05:05:49, tasak wrote:
> On 2016/07/25 12:28:07, drott wrote:
> > Drive by: This looks a bit coarse to me. I am all for memory improvements in
the
> > area of fonts but it makes sense to look at this in a bit more detail. (See
some
> > of my previous CLs, calling FontCache::invalidate on low memory, zero copy
font
> > blob access.)
>
> > We should find a way to contain unbounded growth of the glyphToBoundsMap, or
try
> > to remove it by accessing the Skia-cached information faster.
>
> I think, the way to contain unbounded growth is required for throttle of
MemoryCoordinator.
> This is for MemoryCoordinator's purge. I think, this is different.

My thinking was: The overall goal is to reduce excessive memory consumption. If
our font code requires less memory, there is less memory pressure, less need for
purging. But maybe you could explain a bit more about MemoryCoordinator's purge
and when it is invoked? My intention was to just take a broader perspective on
what we could do in the font code to improve our memory consumption patterns.
Purging reduces memory only temporarily.

Purging the HarfBuzzFace may be safe, but after the zero copy HarfBuzzFace table
access change in https://codereview.chromium.org/2080243002, purging the
HarfBuzzFace object does not release any significant amounts of memory any more
since HarfBuzz and HarfBuzzFace do not take copies of font tables any more.

The larger font blobs are mostly held by SkStreamAsset objects that are owned by
SkTypefaces. But currently the code is not ready for re-creating SkTypefaces,
and also: The underlying font blob data is ref counted - which means it doesn't
go away until the very last SkTypeface referencing it is gone.

FontPlatformData (which holds SkTypefaces) and SimpleFontData (which holds
FontPlatformData objects) are held in FontCache and FontDataCache and reference
each other. The font memory management is a bit of a tangled mess which I am
trying to untangle and get better control over. Help with that is definitely
welcome.

> > On a larger scale: It makes sense to have better font lifecycle management
and
> > somehow create a link between Oilpan and the fonts that are actually in use
in
> > order to avoid unbounded growth.
>
> I would like to ask what's "actually in use". If a renderer is invisible
(background), the fonts are not used.

The fonts are used as long as there are layout operations. And as long as there
are any calls to Font.h API. I believe even in background tabs, layout happens
when there are DOM manipulations - so the fonts are still active, even if there
is not necessarily any painting.

> So we cannot purge? Or if critical memory notification is invoked, we cannot
release such caches?
> I think, the patch is not for lifecycle management. I agree that the change
affects CPU performance. But if we have no memory, we should keep such cache?

I landed a CL https://codereview.chromium.org/2081333004 to invalidate font
caches on memory pressure, this means all unused fonts and their underlying font
blob data is removed.

As soon as we have better control about which fonts are still used, i.e.
referenced by CSS and used in layout, we can further reduce memory consumption.
If aggressive purging is needed, we could also work on temporarily releasing all
SkTypefaces - however I don't see a lot of benefit in that in comparison to the
other suggested improvements.


https://codereview.chromium.org/2178883002/

[har...@chromium.org]

Sorry for lacking the context for this CL. We'll step back and write an
Intent-to-implement to show a big picture of what we're planning here.

The purgeMemory() API is a part of the purge + suspend proposal
(https://docs.google.com/document/d/1thLjq-PYoKWaM_ODDZjojr9VRG6uGnxeL86EaSzYL5k/edit).
It is intended to purge as much memory as possible assuming that the renderer
gets suspended. So it's fine to purge as many fonts as possible aggressively.



https://codereview.chromium.org/2178883002/

[dr...@chromium.org]

On 2016/07/26 at 11:02:02, haraken wrote:

> Sorry for lacking the context for this CL. We'll step back and write an
Intent-to-implement to show a big picture of what we're planning here.
>
> The purgeMemory() API is a part of the purge + suspend proposal
(https://docs.google.com/document/d/1thLjq-PYoKWaM_ODDZjojr9VRG6uGnxeL86EaSzYL5k/edit).
It is intended to purge as much memory as possible assuming that the renderer
gets suspended. So it's fine to purge as many fonts as possible aggressively.

Thanks for the context, some of the results from that study look pretty good.
However, please consider my previous comments regarding this particular CL: I
don't think purging HarfBuzzFace gains any memory after the zero copy change and
inactive SkTypefaces are already removed by purging the FontCache as much as
currently possible. Moving to a system where we can remove SkTypefaces (and thus
free their owned font blobds) from FontPlatformData and resurrecting those
requires further changes and SkTypeface lifecycle management in
FontPlatformData.


https://codereview.chromium.org/2178883002/

[har...@chromium.org]

Yes, thanks for the feedback. We're not planning to be pushy about what we
prototyped in https://codereview.chromium.org/2130683002/. The CL might be doing
something too aggressive, so we plan to land it incrementally confirming that
each piece makes sense :)


https://codereview.chromium.org/2178883002/

[esp...@chromium.org]

Thus patch is just dead code Id rather not do that. Instead lets make this more
framework like. We should add a directory in platform like memory and a new pure
virtual class like MemoryPurgable with a pure virtual method purgeMemory().

We should then add a coordinator class that keeps a Vector of things that are of
type MemoryPurgable. When we want to purge we call the method on each item in
the class and then suspend.

Any patch that adds new one of these needs unit tests too.

https://codereview.chromium.org/2178883002/

[ta...@google.com]

On 2016/07/26 13:12:00, drott wrote:
> On 2016/07/26 at 11:02:02, haraken wrote:
>
> > Sorry for lacking the context for this CL. We'll step back and write an
> Intent-to-implement to show a big picture of what we're planning here.
> >
> > The purgeMemory() API is a part of the purge + suspend proposal
>
(https://docs.google.com/document/d/1thLjq-PYoKWaM_ODDZjojr9VRG6uGnxeL86EaSzYL5k/edit).
> It is intended to purge as much memory as possible assuming that the renderer
> gets suspended. So it's fine to purge as many fonts as possible aggressively.
>
> Thanks for the context, some of the results from that study look pretty good.
> However, please consider my previous comments regarding this particular CL: I
> don't think purging HarfBuzzFace gains any memory after the zero copy change
and
> inactive SkTypefaces are already removed by purging the FontCache as much as
> currently possible. Moving to a system where we can remove SkTypefaces (and
thus
> free their owned font blobds) from FontPlatformData and resurrecting those
> requires further changes and SkTypeface lifecycle management in
> FontPlatformData.

I think, current FontCache()->invalidate() checks whether each font is used or
not.
However, if a renderer is background's one, its layout, rendering, and painting
are throttled. Javascript is running very very slowly.
So I think, we can treat as if the fonts are not used. So I think, it is
possible to purge all fonts' harfBuzzFace, and so on.

Talking about SkTypefaces, I agree that the Zero Copy patch is valuable.
However, in my context, I also want to purge skia's code.
If such removing duplicate SkTypefaces reduces much memory, it is also valuable
to purge the original SkTypefaces.
Since background tabs are not layouted, rendered and painted (in the most case),
it is possible to purge.

What do you think about this?






https://codereview.chromium.org/2178883002/

[ta...@google.com]

Thank you, esprehn@.

I agree with you. I will ask bashi@ and memory coordinator guys, and will
recreate the patch.


https://codereview.chromium.org/2178883002/

[dr...@chromium.org]

On 2016/07/27 at 02:25:12, tasak wrote:

> I think, current FontCache()->invalidate() checks whether each font is used or
not.

Yes, according to the definition of "active/inactive" in the FontCache. This
code is from 2013 and was mainly used with the simple text code path. It's on of
the reasons we have such a hard time reasoning about the lifecycle about our
font objects. The active/inactive definition there is not the source of truth
for which fonts are live in the document. I've tried to fix the usage counting
in the FontCache and FontDataCache back in sync for the complex text code path,
but there are still mismatches. So we cannot currently rely on the
"active/inactive" definition of the FontDataCache.

> However, if a renderer is background's one, its layout, rendering, and
painting are throttled. Javascript is running very very slowly.
> So I think, we can treat as if the fonts are not used. So I think, it is
possible to purge all fonts' harfBuzzFace, and so on.

The definition of font is used: Almost any call to Font.h public methods
requires the SkTypefaces and the word cache to be active. Whether functions of
that API are used more frequently or less frequently when throttled does not
really make a difference then. We cannot treat it "as if fonts are not used"
without ensuring that there are no calls to Font.h API.

HarfBuzzFace objects are a thin shell and do not have large amounts of memory
attached to them any more. That was what the zero copy change did.

> Talking about SkTypefaces, I agree that the Zero Copy patch is valuable.
However, in my context, I also want to purge skia's code.
> If such removing duplicate SkTypefaces reduces much memory, it is also
valuable to purge the original SkTypefaces.
> Since background tabs are not layouted, rendered and painted (in the most
case), it is possible to purge.

> What do you think about this?

What do you mean by "purge Skia's code"? Are you sure there are no calls to
layout and more specifically no calls to Font API in background tabs?

"Where is the actual memory usage?" in the font code is a difficult question,
which I have been studying and trying to answer for a while now. Font blobs are
held in SkStreamAssets which are owned by SkTypefaces. Those SkStreamAssets are
ref counted and difficult to completely unref down to 0 with the current
FontCache architecture, especially in single-process mode (e.g. Android
webview). So in terms of memory consumption, not much changes by deleting a
single SkTypeface since the underlying SkStreamAsset might still have a refcount
> 0. Also, we used to have copies of parts of these font blobs, i.e. single
tables copied, in HarfBuzzFace, which we dont't have anymore, so there was a big
memory saving there already.

As I said before in this CL: Currently our code is not ready to purge
SkTypefaces and recreate them. The code is assuming that the SkTypeface of a
FontPlatformData is not null. So in short: Purging HarfBuzzFace objects does not
gain any significant memory cleanup after the zero copy patch. Purging
SkTypefaces is not safe without further work.


https://codereview.chromium.org/2178883002/

[ta...@google.com]

Thank you for explanation.

On 2016/07/27 07:17:28, drott wrote:
> On 2016/07/27 at 02:25:12, tasak wrote:
>
> > I think, current FontCache()->invalidate() checks whether each font is used
or
> not.
>
> Yes, according to the definition of "active/inactive" in the FontCache. This
> code is from 2013 and was mainly used with the simple text code path. It's on
of
> the reasons we have such a hard time reasoning about the lifecycle about our
> font objects. The active/inactive definition there is not the source of truth
> for which fonts are live in the document. I've tried to fix the usage counting
> in the FontCache and FontDataCache back in sync for the complex text code
path,
> but there are still mismatches. So we cannot currently rely on the
> "active/inactive" definition of the FontDataCache.

I think, I misunderstand. However, the caches are recreated when requested.
So it seems to be purgeable, doesn't it?

> The definition of font is used: Almost any call to Font.h public methods
> requires the SkTypefaces and the word cache to be active. Whether functions of
> that API are used more frequently or less frequently when throttled does not
> really make a difference then. We cannot treat it "as if fonts are not used"
> without ensuring that there are no calls to Font.h API.

I think, background renderers don't need to call such APIs... If need, it is
possible to recreate such data.

> HarfBuzzFace objects are a thin shell and do not have large amounts of memory
> attached to them any more. That was what the zero copy change did.

As far as I understand, SkTypefaces are created from Resources. So if we have
resources, we can recreate.
And since HarfBuzFace was large before your patch, SkTypefaces (in skia) is
large.
I think, it is worth trying to purge the SkTypefaces?

I mean, since throttling, some CPU performance regression is allowed --- we have
time for recreating such data.

> What do you mean by "purge Skia's code"? Are you sure there are no calls to
> layout and more specifically no calls to Font API in background tabs?

I would like to say, if we seldom invokes FontAPI in background tabs, such data
would be recreated from font resources when required.

> "Where is the actual memory usage?" in the font code is a difficult question,
> which I have been studying and trying to answer for a while now. Font blobs
are
> held in SkStreamAssets which are owned by SkTypefaces. Those SkStreamAssets
are
> ref counted and difficult to completely unref down to 0 with the current
> FontCache architecture, especially in single-process mode (e.g. Android
> webview). So in terms of memory consumption, not much changes by deleting a
> single SkTypeface since the underlying SkStreamAsset might still have a
refcount
> > 0. Also, we used to have copies of parts of these font blobs, i.e. single
> tables copied, in HarfBuzzFace, which we dont't have anymore, so there was a
big
> memory saving there already.

I agree that your zero copy is a big memory saving.

I would like to say, skia has some purge method to reduce skia's internal font
caches.
However, if blink holds some of the fonts, we cannot reduce memory usage.

> As I said before in this CL: Currently our code is not ready to purge
> SkTypefaces and recreate them. The code is assuming that the SkTypeface of a
> FontPlatformData is not null. So in short: Purging HarfBuzzFace objects does
not
> gain any significant memory cleanup after the zero copy patch. Purging
> SkTypefaces is not safe without further work.

Yeah, so I reverted the code : m_typeface = nullptr. Thank you for explanation.


https://codereview.chromium.org/2178883002/

[ta...@google.com]

So I would like to close this, because most of FontCache's things are not
currently recreated.
Thank you.





https://codereview.chromium.org/2178883002/