[inspector] introduced memory size limit for console message storage

[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org, ulan@chromium.org

Review-Url: https://codereview.chromium.org/2653293003
Cr-Commit-Position: refs/heads/master@{#42780}
Committed: https://chromium.googlesource.com/v8/v8/+/3903817e0eead8f9812ee0e5481379190bfa3410

[kozy, 2017-01-25 23:04:00]

Description was changed from

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org
==========

to

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org
==========

[kozy, 2017-01-25 23:04:00]

kozyatinskiy@chromium.org changed reviewers:
+ hpayer@chromium.org

[kozy, 2017-01-25 23:05:22]

Aleksey or Dmitry please take a look on inspector part.

Hannes, is it fine to use HeapObject::Size to estimate size of the heap object
in the way as it is used in this CL? Does this method work fast?

[kozy, 2017-01-25 23:06:01]

kozyatinskiy@chromium.org changed reviewers:
+ dgozman@chromium.org
- dgozman@chomium.org

[kozy, 2017-01-25 23:06:28]

added Dmitry to the list of reviewers..

[alph, 2017-01-25 23:45:43]

lgtm

https://codereview.chromium.org/2653293003/diff/1/src/inspector/v8-console-me...
File src/inspector/v8-console-message.cc (right):

https://codereview.chromium.org/2653293003/diff/1/src/inspector/v8-console-me...
src/inspector/v8-console-message.cc:474: while (m_estimatedSize +
message->estimatedSize() > maxConsoleMessageV8Size &&
shouldn't you also count current message size?

https://codereview.chromium.org/2653293003/diff/1/src/inspector/v8-console-me...
src/inspector/v8-console-message.cc:475: m_messages.size()) {
nit: !m_messages.empty()

[kozy, 2017-01-26 02:41:38]

all done.

And we definitely should be more effective in storage, e.g. doesn't store
message string if we've copy inside of first argument.

https://codereview.chromium.org/2653293003/diff/1/src/inspector/v8-console-me...
File src/inspector/v8-console-message.cc (right):

https://codereview.chromium.org/2653293003/diff/1/src/inspector/v8-console-me...
src/inspector/v8-console-message.cc:474: while (m_estimatedSize +
message->estimatedSize() > maxConsoleMessageV8Size &&
On 2017/01/25 23:45:43, alph wrote:
> shouldn't you also count current message size?

m_estimatedSize is current storage size, messages->estimatedSize is size of new
(current) message.
Looks correct to me.

https://codereview.chromium.org/2653293003/diff/1/src/inspector/v8-console-me...
src/inspector/v8-console-message.cc:475: m_messages.size()) {
On 2017/01/25 23:45:43, alph wrote:
> nit: !m_messages.empty()

Done.

[kozy, 2017-01-26 02:41:44]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 02:41:48]

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, 2017-01-26 03:18:54]

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

[commit-bot: I haz the power, 2017-01-26 03:18:55]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-01-26 22:45:53]

Description was changed from

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org
==========

to

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org, ulan@chomium.org
==========

[kozy, 2017-01-26 22:45:53]

kozyatinskiy@chromium.org changed reviewers:
+ ulan@chomium.org

[kozy, 2017-01-26 22:46:22]

Description was changed from

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org, ulan@chomium.org
==========

to

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org, ulan@chromium.org
==========

[kozy, 2017-01-26 22:46:22]

kozyatinskiy@chromium.org changed reviewers:
+ ulan@chromium.org
- ulan@chomium.org

[kozy, 2017-01-26 22:47:54]

Ulan, please take a look on Size usage.

[ulan, 2017-01-30 10:50:25]

lgtm modulo comments

https://codereview.chromium.org/2653293003/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2653293003/diff/20001/src/api.cc#newcode9345
src/api.cc:9345: if (object->IsSmi()) return 4;
Smi uses kPointerSize bytes.

https://codereview.chromium.org/2653293003/diff/20001/src/api.cc#newcode9346
src/api.cc:9346: if (!object->IsHeapObject()) return 0;
This branch is redundant. You can assert here that object is a heap object

[kozy, 2017-01-30 16:13:07]

thank you!

all done.

https://codereview.chromium.org/2653293003/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2653293003/diff/20001/src/api.cc#newcode9345
src/api.cc:9345: if (object->IsSmi()) return 4;
On 2017/01/30 10:50:25, ulan (slow response travel) wrote:
> Smi uses kPointerSize bytes.

Done.

https://codereview.chromium.org/2653293003/diff/20001/src/api.cc#newcode9346
src/api.cc:9346: if (!object->IsHeapObject()) return 0;
On 2017/01/30 10:50:25, ulan (slow response travel) wrote:
> This branch is redundant. You can assert here that object is a heap object

Done.

[kozy, 2017-01-30 16:28:58]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-01-30 16:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org,
ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/2653293003/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-01-30 16:29:00]

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, 2017-01-30 17:05:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485793738090720,
"parent_rev": "475b455b6f67cc825d02641499ab4558f83d6d9a", "commit_rev":
"3903817e0eead8f9812ee0e5481379190bfa3410"}

[commit-bot: I haz the power, 2017-01-30 17:06:04]

Description was changed from

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org, ulan@chromium.org
==========

to

==========
[inspector] introduced memory size limit for console message storage

Without this CL we have only limit for amount of console messages and if user
are dumping a huge messages we pretty soon run out of memory.
So let's introduce limit for memory consumption it would help chromium and
Node.js as well.

BUG=chromium:671489
R=dgozman@chomium.org,alph@chromium.org, hpayer@chromium.org, ulan@chromium.org

Review-Url: https://codereview.chromium.org/2653293003
Cr-Commit-Position: refs/heads/master@{#42780}
Committed:
https://chromium.googlesource.com/v8/v8/+/3903817e0eead8f9812ee0e5481379190bf...
==========

[commit-bot: I haz the power, 2017-01-30 17:06:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/3903817e0eead8f9812ee0e5481379190bf...