Allow self_size to be larger than 2GB in heap snapshots.

Allow self_size to be larger than 2GB in heap snapshots.

LOG=N
R=dslomov@chromium.org, yurys@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=19445

[alph, 2014-02-14 09:46:37]

ptal

[loislo, 2014-02-14 13:26:02]

https://codereview.chromium.org/166383002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/166383002/diff/1/src/api.cc#newcode6974
src/api.cc:6974: size_t size = ToInternal(this)->self_size();
please introduce GetShallowSize with size_t return value and deprecate this
method.

[alph, 2014-02-17 16:57:51]

ptal

https://chromiumcodereview.appspot.com/166383002/diff/1/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/166383002/diff/1/src/api.cc#newcode6974
src/api.cc:6974: size_t size = ToInternal(this)->self_size();
On 2014/02/14 13:26:02, loislo wrote:
> please introduce GetShallowSize with size_t return value and deprecate this
> method.

Done.

[yurys, 2014-02-18 08:03:25]

lgtm

https://chromiumcodereview.appspot.com/166383002/diff/60001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/166383002/diff/60001/src/api.cc#newcod...
src/api.cc:6975: CHECK(size <= static_cast<size_t>(internal::kMaxInt));
As I understand this check may well fail which is the reason you introduce
GetShallowSize. Should we remove it?

[alph, 2014-02-18 12:21:30]

https://chromiumcodereview.appspot.com/166383002/diff/60001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/166383002/diff/60001/src/api.cc#newcod...
src/api.cc:6975: CHECK(size <= static_cast<size_t>(internal::kMaxInt));
On 2014/02/18 08:03:25, yurys wrote:
> As I understand this check may well fail which is the reason you introduce
> GetShallowSize. Should we remove it?

I introduced GetShallowSize to avoid breaking the API.
AFAIK with Blink client there's no objects of size greater than 2GB, so this
check should never fail. For other clients I think it's better to fail rather
than return a bogus value. Nevertheless this function is deprecated and will be
removed.

[alph, 2014-02-18 12:21:53]

Dmitry could you please take a look.

[Dmitry Lomov (no reviews), 2014-02-18 12:40:41]

lgtm (a small warning re windows build)

https://chromiumcodereview.appspot.com/166383002/diff/60001/test/cctest/test-...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/166383002/diff/60001/test/cctest/test-...
test/cctest/test-heap-profiler.cc:237: CHECK_NE(0, x->GetShallowSize());
IIRC, you'll need static_cast<int> to compile on Windows - make sure you run on
a try bot before landing (also below)

[alph, 2014-02-18 13:21:17]

https://chromiumcodereview.appspot.com/166383002/diff/60001/test/cctest/test-...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/166383002/diff/60001/test/cctest/test-...
test/cctest/test-heap-profiler.cc:237: CHECK_NE(0, x->GetShallowSize());
On 2014/02/18 12:40:41, Dmitry Lomov (chromium) wrote:
> IIRC, you'll need static_cast<int> to compile on Windows - make sure you run
on
> a try bot before landing (also below)

Thank you! Added casts to be on the safe side.

[alph, 2014-02-18 13:22:17]

Committed patchset #3 manually as r19445 (presubmit successful).