Report PeriodicWave memory usage to v8 so GC can be properly scheduled

Report PeriodicWave memory usage to v8 so GC can be properly scheduled

A PeriodicWave object can use half a MB and v8 needs to know about that
or it will not schedule garbage collects when memory usage increases.

BUG=578351
Committed: https://crrev.com/1619b2cef936f9937c9cccf09518a49db2bf80a7
Cr-Commit-Position: refs/heads/master@{#371777}

[Daniel Bratell, 2016-01-25 17:30:23]

bratell@opera.com changed reviewers:
+ rtoy@chromium.org

[Daniel Bratell, 2016-01-25 17:30:23]

rtoy, can you please take a look?

[Raymond Toy, 2016-01-25 17:37:45]

On 2016/01/25 17:30:23, Daniel Bratell wrote:
> rtoy, can you please take a look?

lgtm, but a couple of questions.

Can we add a layout test like the example from the bug?  We shouldn't crash if
v8 is collecting the tables.

I don't know enough about the correct usage of
AdjustAmountOfExternalAllocatedMemory.  In AudioBuffer.cpp, there used to be
calls for this to inform v8 about the memory used.  These were removed some time
ago (I don't know why), but I assume it's done in the v8 wrapper instead.

[hongchan, 2016-01-25 17:43:58]

On 2016/01/25 17:37:45, Raymond Toy wrote:
> On 2016/01/25 17:30:23, Daniel Bratell wrote:
> > rtoy, can you please take a look?
> 
> lgtm, but a couple of questions.
> 
> Can we add a layout test like the example from the bug?  We shouldn't crash if
> v8 is collecting the tables.
> 
> I don't know enough about the correct usage of
> AdjustAmountOfExternalAllocatedMemory.  In AudioBuffer.cpp, there used to be
> calls for this to inform v8 about the memory used.  These were removed some
time
> ago (I don't know why), but I assume it's done in the v8 wrapper instead.

AdjustAmountOfExternalAllocatedMemory is pretty rare. If the memory usage is
still high as reported in the issue comment, perhaps this is not the right way
to do it. We need some insight from V8 team.

[Daniel Bratell, 2016-01-26 10:57:51]

On 2016/01/25 17:43:58, hoch wrote:
> On 2016/01/25 17:37:45, Raymond Toy wrote:
> > On 2016/01/25 17:30:23, Daniel Bratell wrote:
> > > rtoy, can you please take a look?
> > 
> > lgtm, but a couple of questions.
> > 
> > Can we add a layout test like the example from the bug?  We shouldn't crash
if
> > v8 is collecting the tables.
> > 
> > I don't know enough about the correct usage of
> > AdjustAmountOfExternalAllocatedMemory.  In AudioBuffer.cpp, there used to be
> > calls for this to inform v8 about the memory used.  These were removed some
> time
> > ago (I don't know why), but I assume it's done in the v8 wrapper instead.
> 
> AdjustAmountOfExternalAllocatedMemory is pretty rare. If the memory usage is
> still high as reported in the issue comment, perhaps this is not the right way
> to do it. We need some insight from V8 team.

I've sent a mail to some v8 people but asking around people don't know a
different way of doing this. The AudioBuffer patch rtoy mentioned, I guess it
was https://codereview.chromium.org/1183763003 and it seems similar but
different. I'm not really sure what it's doing, nor how the old code worked.

The high memory usage might be the memory grows so very quick. 0.5 MB per call
to a DOM function is very fast, and with V8 tuned to keep gc latency down, it
doesn't react directly. I can see that the calls to adjust external memory
triggers incremental garbage collects once the memory has reached 192 MB (a
hardcoded constant). 

It might be a bug in v8 that it is not more aggressive and I'll discuss it with
hpayer and ulan. They work in my timezone I think so I might get some info later
today. Maybe. :-)

[jochen (gone - plz use gerrit), 2016-01-26 15:33:55]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-01-26 15:33:56]

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp:174:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(delta);
mainThreadIsolate()

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/PeriodicWave.h (right):

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/PeriodicWave.h:51: ~PeriodicWave();
needs to be virtual

[Daniel Bratell, 2016-01-26 16:19:13]

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp:174:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(delta);
On 2016/01/26 15:33:55, jochen wrote:
> mainThreadIsolate()

Is that important?
modules.dll doesn't know how to call that function:

1>  [2/38] LINK_EMBED(DLL) modules.dll
1>  FAILED: L:\src\depot_tools\python276_bin\python.exe gyp-win-tool
link-with-manifests environment.x86 True modules.dll
"L:\src\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper
environment.x86 False link.exe /nologo /IMPLIB:modules.dll.lib /DLL
/OUT:modules.dll @modules.dll.rsp" 2 mt.exe rc.exe
"obj\third_party\WebKit\Source\modules\modules.modules.dll.intermediate.manifest"
obj\third_party\WebKit\Source\modules\modules.modules.dll.generated.manifest
1>modules.PeriodicWave.obj : error LNK2019: unresolved external symbol "class
v8::Isolate * __cdecl blink::mainThreadIsolate(void)"
(?mainThreadIsolate@blink@@YAPAVIsolate@v8@@XZ) referenced in function "private:
void __thiscall blink::PeriodicWave::adjustV8ExternalMemory(int)"
(?adjustV8ExternalMemory@PeriodicWave@blink@@AAEXH@Z)

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/PeriodicWave.h (right):

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/PeriodicWave.h:51: ~PeriodicWave();
On 2016/01/26 15:33:55, jochen wrote:
> needs to be virtual

Done.

[jochen (gone - plz use gerrit), 2016-01-26 17:11:51]

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/1632753002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp:174:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(delta);
I'm trying to get rid of this method - the embedder should keep track of which
instance of V8 to use.

Anyway, I don't see an easy way to get to the symbol, so it's ok to add it here
:-/

[jochen (gone - plz use gerrit), 2016-01-27 07:31:56]

lgtm

[Daniel Bratell, 2016-01-27 10:45:10]

The CQ bit was checked by bratell@opera.com

[Daniel Bratell, 2016-01-27 10:45:11]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org
Link to the patchset: https://codereview.chromium.org/1632753002/#ps20001
(title: "Added virtual")

[commit-bot: I haz the power, 2016-01-27 10:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632753002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632753002/20001

[commit-bot: I haz the power, 2016-01-27 12:11:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-27 12:12:29]

Description was changed from

==========
Report PeriodicWave memory usage to v8 so GC can be properly scheduled

A PeriodicWave object can use half a MB and v8 needs to know about that
or it will not schedule garbage collects when memory usage increases.

BUG=578351
==========

to

==========
Report PeriodicWave memory usage to v8 so GC can be properly scheduled

A PeriodicWave object can use half a MB and v8 needs to know about that
or it will not schedule garbage collects when memory usage increases.

BUG=578351

Committed: https://crrev.com/1619b2cef936f9937c9cccf09518a49db2bf80a7
Cr-Commit-Position: refs/heads/master@{#371777}
==========

[commit-bot: I haz the power, 2016-01-27 12:12:30]

Patchset 2 (id:??) landed as
https://crrev.com/1619b2cef936f9937c9cccf09518a49db2bf80a7
Cr-Commit-Position: refs/heads/master@{#371777}