Remove 64-bit unclean line from heap size estimation

Remove 64-bit unclean line from heap  size estimation

R=hpayer@chromium.org
BUG=

Committed: https://crrev.com/9ff1f53b085832994f8de496853f06229268df9e
Cr-Commit-Position: refs/heads/master@{#28503}

[Erik Corry Chromium.org, 2015-05-15 14:13:25]

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

[commit-bot: I haz the power, 2015-05-15 14:13:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133243006/1

[commit-bot: I haz the power, 2015-05-15 14:57:48]

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

[commit-bot: I haz the power, 2015-05-15 14:57:49]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2015-05-15 15:08:18]

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

[Hannes Payer (out of office), 2015-05-15 15:08:19]

https://codereview.chromium.org/1133243006/diff/1/src/heap/heap.h
File src/heap/heap.h (left):

https://codereview.chromium.org/1133243006/diff/1/src/heap/heap.h#oldcode1084
src/heap/heap.h:1084: if (total > kMaxInt) return
static_cast<intptr_t>(kMaxInt);
Unfortunatly, an overflow is possible here. jkummerow, what caused the
regression?

[Jakob Kummerow, 2015-05-15 16:13:34]

I don't remember the specifics; this is the CL that introduced that line:
https://codereview.chromium.org/70233010

This just happens to be the boundary between intptr_t and int64_t worlds. On
32-bit platforms, we need to guard against overflow there. Why do you want to
remove the check?

[Erik Corry, 2015-05-19 13:49:02]

On 2015/05/15 16:13:34, Jakob wrote:
> I don't remember the specifics; this is the CL that introduced that line:
> https://codereview.chromium.org/70233010
> 
> This just happens to be the boundary between intptr_t and int64_t worlds. On
> 32-bit platforms, we need to guard against overflow there. Why do you want to
> remove the check?

I want to remove the check because it's totally broken for 64 bit platforms like
node.js that routinely raise the heap size.

This isn't something I want to spend a lot of time on, since we don't have a
repro of a problem in Chrome, but as a compromise we could get rid of this line
on 64 bit where it is obviously completely broken.  New patch set uploaded.

[Erik Corry Chromium.org, 2015-05-19 13:52:53]

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

[commit-bot: I haz the power, 2015-05-19 13:53:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133243006/20001

[Jakob Kummerow, 2015-05-19 14:23:25]

jkummerow@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Jakob Kummerow, 2015-05-19 14:23:25]

Suggestion below.

Sven, do you have an opinion?

https://codereview.chromium.org/1133243006/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1133243006/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1084: if (sizeof(int) < sizeof(int64_t) && total > kMaxInt) {
Isn't "sizeof(int) < sizeof(int64_t)" true on all platforms? How about we change
the original line to:

if (total > std::numeric_limits<intptr_t>::max()) {
  return std::numeric_limits<intptr_t>::max();
}

[Sven Panne, 2015-05-19 14:41:46]

https://codereview.chromium.org/1133243006/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1133243006/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1084: if (sizeof(int) < sizeof(int64_t) && total > kMaxInt) {
On 2015/05/19 14:23:25, Jakob wrote:
> Isn't "sizeof(int) < sizeof(int64_t)" true on all platforms?

Currently, yes (see http://www.unix.org/whitepapers/64bit.html), but there's no
fundamental reason why there can't be an IP64 platform. :-}

> How about we change the original line to:
> 
> if (total > std::numeric_limits<intptr_t>::max()) {
>   return std::numeric_limits<intptr_t>::max();
> }

Looks OK to me, but I'm really puzzled by all those types used for sizes here
(int, int64_t, intptr_t), this looks really scary and error-prone. Perhaps we
should consistently use (s)size_t or ptrdiff_t? o_O

[commit-bot: I haz the power, 2015-05-19 14:54:06]

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

[commit-bot: I haz the power, 2015-05-19 14:54:07]

Dry run: This issue passed the CQ dry run.

[Erik Corry Chromium.org, 2015-05-19 15:04:18]

https://codereview.chromium.org/1133243006/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1133243006/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1084: if (sizeof(int) < sizeof(int64_t) && total > kMaxInt) {
On 2015/05/19 14:23:25, Jakob wrote:
> Isn't "sizeof(int) < sizeof(int64_t)" true on all platforms? How about we
change

Oh yeah, good point.

> the original line to:
> if (total > std::numeric_limits<intptr_t>::max()) {
>   return std::numeric_limits<intptr_t>::max();
> }

Works for me.

https://codereview.chromium.org/1133243006/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1084: if (sizeof(int) < sizeof(int64_t) && total > kMaxInt) {
On 2015/05/19 14:41:46, Sven Panne wrote:
> On 2015/05/19 14:23:25, Jakob wrote:
> > Isn't "sizeof(int) < sizeof(int64_t)" true on all platforms?
> 
> Currently, yes (see http://www.unix.org/whitepapers/64bit.html), but there's
no
> fundamental reason why there can't be an IP64 platform. :-}
> 
> > How about we change the original line to:
> > 
> > if (total > std::numeric_limits<intptr_t>::max()) {
> >   return std::numeric_limits<intptr_t>::max();
> > }
> 
> Looks OK to me, but I'm really puzzled by all those types used for sizes here
> (int, int64_t, intptr_t), this looks really scary and error-prone. Perhaps we
> should consistently use (s)size_t or ptrdiff_t? o_O

Yes, see the TODO.  There's no difference between size_t and uintptr_t on any
sensible platform, but on a theoretical segmented platform uintptr_t would be
big enough to hold the size of all objects in all segments, whereas size_t might
only be big enough for the biggest object in one segment.

[Erik Corry Chromium.org, 2015-05-20 09:11:04]

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

[commit-bot: I haz the power, 2015-05-20 09:11:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133243006/40001

[Jakob Kummerow, 2015-05-20 09:24:38]

Patch set 3 LGTM.

[commit-bot: I haz the power, 2015-05-20 09:39:35]

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

[commit-bot: I haz the power, 2015-05-20 09:39:36]

Dry run: This issue passed the CQ dry run.

[Erik Corry Chromium.org, 2015-05-20 09:39:49]

The CQ bit was checked by erikcorry@chromium.org

[commit-bot: I haz the power, 2015-05-20 09:40:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133243006/40001

[commit-bot: I haz the power, 2015-05-20 09:44:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-20 09:44:44]

Patchset 3 (id:??) landed as
https://crrev.com/9ff1f53b085832994f8de496853f06229268df9e
Cr-Commit-Position: refs/heads/master@{#28503}