[Experiment] purge-and-suspend

[Experiment] purge-and-suspend

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[tasak, 2016-07-07 07:03:45]

Description was changed from

==========
[Experiment] purge-and-suspend

BUG=
==========

to

==========
[Experiment] purge-and-suspend

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[tasak, 2016-07-07 07:03:46]

Description was changed from

==========
[Experiment] purge-and-suspend

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
[Experiment] purge-and-suspend

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[tasak, 2016-07-07 07:05:41]

tasak@google.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[haraken, 2016-07-07 09:12:25]

The purging targets look good to me (at least as a starting point).

Let's discuss how the purging API should look like with bashi@.

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp (left):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:40:
WTF::Partitions::decommitFreeableMemory();

Where is this called after your CL?

[tasak, 2016-07-07 09:30:59]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp (left):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:40:
WTF::Partitions::decommitFreeableMemory();
On 2016/07/07 09:12:25, haraken wrote:
> 
> Where is this called after your CL?

Placed in WebMemoryCoordinator::onMemoryPressure(WebMemoryPressureLevel
pressureLevel)  after calling all purge / prune methods.

[sof, 2016-07-07 09:35:18]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-07-07 09:35:19]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1204:
discardPages(startOfGap + sizeof(FreeListEntry), headerAddress);
Why drop the check for low-end devices?

[tasak, 2016-07-08 03:56:45]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1204:
discardPages(startOfGap + sizeof(FreeListEntry), headerAddress);
On 2016/07/07 09:35:19, sof wrote:
> Why drop the check for low-end devices?

To reduce resident_set_bytes, pss, or private_dirty.

However, I agree that the change is a quick hack.
I will revert this change. Instead, I will add some method / function for
discarding oilpan's unused pages which will be invoked when memory pressure is
notified.

[bashi, 2016-07-08 04:16:23]

Yeah, let's discuss later how we should trigger these purging logic.

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebMemoryCoordinator.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebMemoryCoordinator.cpp:25:
WTF::Partitions::decommitFreeableMemory();
I think these should be in core::MemoryCoordinator. My understanding is that
web/ shouldn't contain any logic.

https://codereview.chromium.org/2130683002/diff/1/tools/perf/page_sets/memory...
File tools/perf/page_sets/memory_test.py (right):

https://codereview.chromium.org/2130683002/diff/1/tools/perf/page_sets/memory...
tools/perf/page_sets/memory_test.py:1: # Copyright 2014 The Chromium Authors.
All rights reserved.
Are you going to add this?

[tasak, 2016-07-08 06:47:23]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebMemoryCoordinator.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebMemoryCoordinator.cpp:25:
WTF::Partitions::decommitFreeableMemory();
On 2016/07/08 04:16:22, bashi1 wrote:
> I think these should be in core::MemoryCoordinator. My understanding is that
> web/ shouldn't contain any logic.

Since I heard that core and modules will be the same shared library, I think,
it's ok.
(I mean, if we want to keep core and modules different shared libraries, how can
we invoke core logic and module logic?)

However, I think, decommitFreeableMemory should be invoked after invoking all
other purge method.
If we move the code to core, we need to add such comment.

https://codereview.chromium.org/2130683002/diff/1/tools/perf/page_sets/memory...
File tools/perf/page_sets/memory_test.py (right):

https://codereview.chromium.org/2130683002/diff/1/tools/perf/page_sets/memory...
tools/perf/page_sets/memory_test.py:1: # Copyright 2014 The Chromium Authors.
All rights reserved.
On 2016/07/08 04:16:22, bashi1 wrote:
> Are you going to add this?

No. This code is just for explaining how I measured renderer memory usages.

[bashi, 2016-07-08 07:09:09]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebMemoryCoordinator.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebMemoryCoordinator.cpp:25:
WTF::Partitions::decommitFreeableMemory();
On 2016/07/08 06:47:23, tasak wrote:
> On 2016/07/08 04:16:22, bashi1 wrote:
> > I think these should be in core::MemoryCoordinator. My understanding is that
> > web/ shouldn't contain any logic.
> 
> Since I heard that core and modules will be the same shared library, I think,
> it's ok.
> (I mean, if we want to keep core and modules different shared libraries, how
can
> we invoke core logic and module logic?)
In this case I think we can add a callback in core/MemoryCoordinator to inverse
core -> modules dependency.
> 
> However, I think, decommitFreeableMemory should be invoked after invoking all
> other purge method.
> If we move the code to core, we need to add such comment.
>  

My concern is code organization. These logic should live in core/. web/ should
be as thin as possible. Rationale:
https://codereview.chromium.org/1415663004/diff/60001/third_party/WebKit/Sour...

I may miss something but why can't we call it at the bottom of
MemoryCoordinator::onMemoryPressure()?

[sof, 2016-07-08 07:14:30]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1204:
discardPages(startOfGap + sizeof(FreeListEntry), headerAddress);
On 2016/07/08 03:56:45, tasak wrote:
> On 2016/07/07 09:35:19, sof wrote:
> > Why drop the check for low-end devices?
> 
> To reduce resident_set_bytes, pss, or private_dirty.
> 
> However, I agree that the change is a quick hack.
> I will revert this change. Instead, I will add some method / function for
> discarding oilpan's unused pages which will be invoked when memory pressure is
> notified.
> 

That would be better; what's here now will seriously upset paging behavior (and
was an quick & expedient change put in to counter an RSS regression seen on
lower-end devices.)

[tasak, 2016-07-19 03:15:18]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 03:15:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 03:15:43]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-19 04:45:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 04:45:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tasak, 2016-07-19 08:22:51]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1204:
discardPages(startOfGap + sizeof(FreeListEntry), headerAddress);
On 2016/07/08 07:14:29, sof wrote:
> On 2016/07/08 03:56:45, tasak wrote:
> > On 2016/07/07 09:35:19, sof wrote:
> > > Why drop the check for low-end devices?
> > 
> > To reduce resident_set_bytes, pss, or private_dirty.
> > 
> > However, I agree that the change is a quick hack.
> > I will revert this change. Instead, I will add some method / function for
> > discarding oilpan's unused pages which will be invoked when memory pressure
is
> > notified.
> > 
> 
> That would be better; what's here now will seriously upset paging behavior
(and
> was an quick & expedient change put in to counter an RSS regression seen on
> lower-end devices.)

I think, I need, Oilpan::discardUnusedPages or something. I would like to
discuss with haraken@.

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebMemoryCoordinator.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebMemoryCoordinator.cpp:25:
WTF::Partitions::decommitFreeableMemory();
On 2016/07/08 07:09:09, bashi1 wrote:
> On 2016/07/08 06:47:23, tasak wrote:
> > On 2016/07/08 04:16:22, bashi1 wrote:
> > > I think these should be in core::MemoryCoordinator. My understanding is
that
> > > web/ shouldn't contain any logic.
> > 
> > Since I heard that core and modules will be the same shared library, I
think,
> > it's ok.
> > (I mean, if we want to keep core and modules different shared libraries, how
> can
> > we invoke core logic and module logic?)
> In this case I think we can add a callback in core/MemoryCoordinator to
inverse
> core -> modules dependency.
> > 
> > However, I think, decommitFreeableMemory should be invoked after invoking
all
> > other purge method.
> > If we move the code to core, we need to add such comment.
> >  
> 
> My concern is code organization. These logic should live in core/. web/ should
> be as thin as possible. Rationale:
>
https://codereview.chromium.org/1415663004/diff/60001/third_party/WebKit/Sour...
> 
> I may miss something but why can't we call it at the bottom of
> MemoryCoordinator::onMemoryPressure()?

Done.

[haraken, 2016-07-19 20:20:22]

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/2130683002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1204:
discardPages(startOfGap + sizeof(FreeListEntry), headerAddress);
On 2016/07/19 08:22:51, tasak wrote:
> On 2016/07/08 07:14:29, sof wrote:
> > On 2016/07/08 03:56:45, tasak wrote:
> > > On 2016/07/07 09:35:19, sof wrote:
> > > > Why drop the check for low-end devices?
> > > 
> > > To reduce resident_set_bytes, pss, or private_dirty.
> > > 
> > > However, I agree that the change is a quick hack.
> > > I will revert this change. Instead, I will add some method / function for
> > > discarding oilpan's unused pages which will be invoked when memory
pressure
> is
> > > notified.
> > > 
> > 
> > That would be better; what's here now will seriously upset paging behavior
> (and
> > was an quick & expedient change put in to counter an RSS regression seen on
> > lower-end devices.)
> 
> I think, I need, Oilpan::discardUnusedPages or something. I would like to
> discuss with haraken@.

This makes sense.

Currently discardPages() is called only on low-end devices
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/...)
but we can change it so that discardPages() is also called in a GC triggered by
memory pressures.

[tasak, 2016-07-20 03:41:58]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 03:42:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 03:42:39]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[tasak, 2016-07-20 03:53:51]

Patchset #3 (id:40001) has been deleted

[tasak, 2016-07-20 03:54:09]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 03:54:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 03:54:48]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-20 05:54:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 05:54:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tasak, 2016-07-20 08:10:53]

tasak@google.com changed reviewers:
+ esprehn@chromium.org

[tasak, 2016-07-20 08:10:54]

Hi esprehn,

Would you review this CL from the viewpoint of what should be purged and what
should not be purged?

I added some purge-memory methods and modified MemoryCoordinator to invoke the
methods when CRITICAL memory pressure notification is notified.

[esprehn, 2016-08-02 00:34:08]

On 2016/07/20 at 08:10:54, tasak wrote:
> Hi esprehn,
> 
> Would you review this CL from the viewpoint of what should be purged and what
should not be purged?
> 
> I added some purge-memory methods and modified MemoryCoordinator to invoke the
methods when CRITICAL memory pressure notification is notified.

Yeah I'll look this over. :)

[esprehn, 2016-08-05 23:00:58]

Where is the suspend part? You hook up a bunch of this stuff to the memory
pressure listener, but without suspend we're just making the page *super* slow
as it tries to restore all the data structures after the notification.

https://codereview.chromium.org/2130683002/diff/60001/cc/raster/staging_buffe...
File cc/raster/staging_buffer_pool.cc (right):

https://codereview.chromium.org/2130683002/diff/60001/cc/raster/staging_buffe...
cc/raster/staging_buffer_pool.cc:200: base::allocator::ReleaseFreeMemory();
is this is very strange inside the cc/raster code... why would StagingBufferPool
call into base::allocator? If we're supposed to do this the MemoryCoordinator
should do it for you automatically after calling the OnMemoryPressure callbacks

https://codereview.chromium.org/2130683002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/2130683002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:305: base::allocator::ReleaseFreeMemory();
yeah...

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:41:
memoryCache()->pruneAll();
Would it be possible to independently hook up these sub systems instead of
making this one know about every feature in the engine?

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:47:
CompressibleStringImpl::compressAll();
hmm without suspend doing this is pretty bad, it means v8 is going to cause them
to uncompress really soon. Did we solve that yet?

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:48: } else {
else if

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:817: clearResolver();
I think this has some scary side effects, you should double check with rune@,
this also leaves all of the ScopedStyleResolvers. It's going to make us recalc
the entire document I think too.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:819:
m_sheetToTextCache.clear();
This code doesn't really make sense, you're leaving around all of the
ScopedStyleResolvers and the CSSStyleSheet which were created from these
strings. Are you sure this actually saves memory? It shouldn't, the cache here
is just for looking up stuff the rest of the DOM is already using

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4307:
rootGraphicsLayer->getPaintController().invalidateAll();
This is only the main graphics layer, what about all of the other ones? Are we
sure this does the right thing?

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (left):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:334: if (m_visibilityState ==
PageVisibilityStateHidden) {
is this actually enabled today?

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp:352:
m_harfBuzzFace = nullptr;
Are we sure this is safe? drott@'s comments on the other patch make it sound bad

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1204:
discardPages(startOfGap + sizeof(FreeListEntry), headerAddress);
Why is it okay to remove the LowEndDevice comments and checks?

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:119: in =
std::string(reinterpret_cast<const char*>(m_string.characters8()),
originalContentSizeInBytes());
ouch this is pretty bad it doubles the memory usage of the huge JS strings while
we're in the process of compressing them.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:148:
compression::GzipUncompress(in, &out);
ditto

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CompressibleString.h (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CompressibleString.h:77: unsigned
m_compressedDataSize;
we should probably do this separately. I'm also concerned about the complexity
the current system causes because it assumes JS can run and force an uncompress.
What if we instead hide the compression the DOM?

[esprehn, 2016-08-05 23:06:27]

Hmm the CompressibleString code doesn't make any sense to me, we only have
CompressibleStrings on  ScriptSourceCode and V8StringResource and
ScriptResource, but none of those are ever accessible to JS so the toV8 stuff we
have around it doesn't matter.

ex. any inline scripts or stylesheets are never CompressibleStrings (they need
to be atomized anyway)

In general the CompressibleString system seems to only matter for external
script resources, that seems fine, but if that's true we should be able to
vastly simplify the code around it and instead make it specific to external text
resources.

[haraken, 2016-08-08 03:55:47]

On 2016/08/05 23:06:27, esprehn wrote:
> Hmm the CompressibleString code doesn't make any sense to me, we only have
> CompressibleStrings on  ScriptSourceCode and V8StringResource and
> ScriptResource, but none of those are ever accessible to JS so the toV8 stuff
we
> have around it doesn't matter.
> 
> ex. any inline scripts or stylesheets are never CompressibleStrings (they need
> to be atomized anyway)
> 
> In general the CompressibleString system seems to only matter for external
> script resources, that seems fine, but if that's true we should be able to
> vastly simplify the code around it and instead make it specific to external
text
> resources.

Regarding your two main concerns, let me comment.

* Where's the suspend code? It's already landed on ToT behind a flag. Our plan
is to integrate this CL to the suspension system. You can assume that this CL is
enabled with the suspension.

* Regarding CompressibleString, we decided not to ship it at the moment. The
main reasons are:

- V8 implementation of external strings is broken and it cannot (yet) work with
CompressibleString in some cases
- We're now experimenting with the idea of "(simply) discarding ScriptResources
assuming that they're cached on disk cache". If we can just discard
ScriptResources, that's definitely better than spending lots of CPU cycles on
the compression.

[tasak, 2016-08-08 04:47:37]

Thank you, esprehn.

Currently we are planning purge-and-suspend without CompressibleString.
"suspend" feature has been already implemented by hajimehoshi@. I will use the
code.

https://codereview.chromium.org/2130683002/diff/60001/cc/raster/staging_buffe...
File cc/raster/staging_buffer_pool.cc (right):

https://codereview.chromium.org/2130683002/diff/60001/cc/raster/staging_buffe...
cc/raster/staging_buffer_pool.cc:200: base::allocator::ReleaseFreeMemory();
On 2016/08/05 23:00:57, esprehn wrote:
> is this is very strange inside the cc/raster code... why would
StagingBufferPool
> call into base::allocator? If we're supposed to do this the MemoryCoordinator
> should do it for you automatically after calling the OnMemoryPressure
callbacks

Acknowledged.

Yeah, I agree that MemoryCoordinator should do. Since we have no
MemoryCoordinator, I temporarily added tons of
base::allocator::ReleaseFreeMemory(). I will revert the bad
base::allocator::ReleaseFreeMemory() calls.

https://codereview.chromium.org/2130683002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/2130683002/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:305: base::allocator::ReleaseFreeMemory();
On 2016/08/05 23:00:57, esprehn wrote:
> yeah...

Acknowledged.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:41:
memoryCache()->pruneAll();
On 2016/08/05 23:00:57, esprehn wrote:
> Would it be possible to independently hook up these sub systems instead of
> making this one know about every feature in the engine?

Acknowledged.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:47:
CompressibleStringImpl::compressAll();
On 2016/08/05 23:00:58, esprehn wrote:
> hmm without suspend doing this is pretty bad, it means v8 is going to cause
them
> to uncompress really soon. Did we solve that yet?

Acknowledged.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/MemoryCoordinator.cpp:48: } else {
On 2016/08/05 23:00:57, esprehn wrote:
> else if

Acknowledged.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:817: clearResolver();
On 2016/08/05 23:00:58, esprehn wrote:
> I think this has some scary side effects, you should double check with rune@,
> this also leaves all of the ScopedStyleResolvers. It's going to make us recalc
> the entire document I think too.

Acknowledged.

I would like to ask one thing. Are you talking about "background to foreground"
full recalc?
As far as I understand, background tabs are suspended and need no layout, no
stylerecalc, and no painting.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:819:
m_sheetToTextCache.clear();
On 2016/08/05 23:00:58, esprehn wrote:
> This code doesn't really make sense, you're leaving around all of the
> ScopedStyleResolvers and the CSSStyleSheet which were created from these
> strings. Are you sure this actually saves memory? It shouldn't, the cache here
> is just for looking up stuff the rest of the DOM is already using

Acknowledged.
I agree that basically these caches' sizes are small and that the impact is very
small.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4307:
rootGraphicsLayer->getPaintController().invalidateAll();
On 2016/08/05 23:00:58, esprehn wrote:
> This is only the main graphics layer, what about all of the other ones? Are we
> sure this does the right thing?

I see. I would like to traverse all children and to invoke their
paintController's "invalidateAll()".

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (left):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:334: if (m_visibilityState ==
PageVisibilityStateHidden) {
On 2016/08/05 23:00:58, esprehn wrote:
> is this actually enabled today?

No, not enabled. I manually applied compressibleString patch.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp:352:
m_harfBuzzFace = nullptr;
On 2016/08/05 23:00:58, esprehn wrote:
> Are we sure this is safe? drott@'s comments on the other patch make it sound
bad

Acknowledged.
I think, drott@ is reducing tons of font memory. He has already reduced what I
pointed out.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1204:
discardPages(startOfGap + sizeof(FreeListEntry), headerAddress);
On 2016/08/05 23:00:58, esprehn wrote:
> Why is it okay to remove the LowEndDevice comments and checks?

I don't want to remove the "if" and the comment. This is just for checking
whether the code affects resident_set_bytes or PSS or private_dirty or not.

I will revert the change when we are doing some experiment.
Instead, MemoryCoordinator will discard such unused pages if required.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:119: in =
std::string(reinterpret_cast<const char*>(m_string.characters8()),
originalContentSizeInBytes());
On 2016/08/05 23:00:58, esprehn wrote:
> ouch this is pretty bad it doubles the memory usage of the huge JS strings
while
> we're in the process of compressing them.

Acknowledged.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:148:
compression::GzipUncompress(in, &out);
On 2016/08/05 23:00:58, esprehn wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CompressibleString.h (right):

https://codereview.chromium.org/2130683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CompressibleString.h:77: unsigned
m_compressedDataSize;
On 2016/08/05 23:00:58, esprehn wrote:
> we should probably do this separately. I'm also concerned about the complexity
> the current system causes because it assumes JS can run and force an
uncompress.
> What if we instead hide the compression the DOM?

Acknowledged.