Sync remaining allocation size on in-place backing store recycling.

Sync remaining allocation size on in-place backing store recycling.

When a heap's current allocation point can simply be moved after the tail
object allocation is either freed, expanded or shrunk, make sure the
snapshot'ed m_lastRemainingAllocationSize is then always adjusted at the
same time.

Not doing so risks later re-syncs of m_lastRemaininingAllocationSize to
be invoked on a "bigger" heap than last time, i.e.,
m_lastRemainingAllocationSize < m_remainingAllocationSize, which is an
unexpected state to be in. Simply avoid that state from ever being entered,
by synchronously updating m_lastRemainingAllocationSize.

setRemainingAllocationSize(size_t) handles this, along with updating the
overall heap allocation counts.

R=haraken
BUG=420515
Committed: https://crrev.com/61b83e2ec049a9aa5343ff0072cdcae420d4943b
Cr-Commit-Position: refs/heads/master@{#357292}

[sof, 2015-11-01 12:13:57]

Description was changed from

==========
Sync remaining object size on in-place backing store recycling.

R=
BUG=
==========

to

==========
Sync remaining allocation size on in-place backing store recycling.

When a heap's current allocation point can simply be moved after the tail
object allocation is either freed, expanded or shrunk, make sure the
snapshot'ed m_lastRemainingAllocationSize is then always adjusted at the
same time.

Not doing so risks later re-syncs of m_lastRemaininingAllocationSize to
be invoked on a "bigger" heap than last time, i.e.,
m_lastRemainingAllocationSize < m_remainingAllocationSize, which is an
unexpected state to be in. Simply avoid that state from ever being entered,
by synchronously updating m_lastRemainingAllocationSize.

setRemainingAllocationSize(size_t) handles this, along with updating the
overall heap allocation counts.

R=
BUG=420515
==========

[sof, 2015-11-01 12:18:49]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-11-01 12:18:50]

please take a look.

Going after flaky asserts like,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...

I think the problem is of a longer standing, but made more likely to surface
following https://codereview.chromium.org/1393863004

[haraken, 2015-11-01 17:45:43]

On 2015/11/01 12:18:50, sof wrote:
> please take a look.
> 
> Going after flaky asserts like,
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> 
> I think the problem is of a longer standing, but made more likely to surface
> following https://codereview.chromium.org/1393863004

LGTM, thanks for fixing the flakiness!

The complexity around m_remainingAllocationSize come from the fact that we don't
want to update m_remainingAllocationSize at every allocation (for performance).
Maybe we can stop the performance optimization (i.e., update the remaining
allocation size at every allocation) in ASSERT builds and add asserts to verify
that m_remainingAllocationSize is holding a correct value.

[sof, 2015-11-01 19:35:10]

On 2015/11/01 17:45:43, haraken wrote:
> On 2015/11/01 12:18:50, sof wrote:
> > please take a look.
> > 
> > Going after flaky asserts like,
> > 
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > 
> > I think the problem is of a longer standing, but made more likely to surface
> > following https://codereview.chromium.org/1393863004
> 
> LGTM, thanks for fixing the flakiness!
> 
> The complexity around m_remainingAllocationSize come from the fact that we
don't
> want to update m_remainingAllocationSize at every allocation (for
performance).
> Maybe we can stop the performance optimization (i.e., update the remaining
> allocation size at every allocation) in ASSERT builds and add asserts to
verify
> that m_remainingAllocationSize is holding a correct value.

I thought it was the prohibitive cost of doing the atomic operation of updating
the heap allocation counts for every call to allocate() that brought about the
m_lastRemainingAllocationSize scheme --

 https://codereview.chromium.org/638223003/ +
https://codereview.chromium.org/634243004/

[haraken, 2015-11-01 19:44:20]

On 2015/11/01 19:35:10, sof wrote:
> On 2015/11/01 17:45:43, haraken wrote:
> > On 2015/11/01 12:18:50, sof wrote:
> > > please take a look.
> > > 
> > > Going after flaky asserts like,
> > > 
> > > 
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > > 
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > > 
> > > I think the problem is of a longer standing, but made more likely to
surface
> > > following https://codereview.chromium.org/1393863004
> > 
> > LGTM, thanks for fixing the flakiness!
> > 
> > The complexity around m_remainingAllocationSize come from the fact that we
> don't
> > want to update m_remainingAllocationSize at every allocation (for
> performance).
> > Maybe we can stop the performance optimization (i.e., update the remaining
> > allocation size at every allocation) in ASSERT builds and add asserts to
> verify
> > that m_remainingAllocationSize is holding a correct value.
> 
> I thought it was the prohibitive cost of doing the atomic operation of
updating
> the heap allocation counts for every call to allocate() that brought about the
> m_lastRemainingAllocationSize scheme --
> 
>  https://codereview.chromium.org/638223003/ +
> https://codereview.chromium.org/634243004/

Yes, so my suggestion is to do the atomic operation only in ASSERT builds and
add asserts to verify that m_lastRemainingAllocationSize is keeping track of a
correct value.

[sof, 2015-11-01 19:48:58]

On 2015/11/01 19:44:20, haraken wrote:
> On 2015/11/01 19:35:10, sof wrote:
> > On 2015/11/01 17:45:43, haraken wrote:
> > > On 2015/11/01 12:18:50, sof wrote:
> > > > please take a look.
> > > > 
> > > > Going after flaky asserts like,
> > > > 
> > > > 
> > > >
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > > > 
> > > >
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > > > 
> > > > I think the problem is of a longer standing, but made more likely to
> surface
> > > > following https://codereview.chromium.org/1393863004
> > > 
> > > LGTM, thanks for fixing the flakiness!
> > > 
> > > The complexity around m_remainingAllocationSize come from the fact that we
> > don't
> > > want to update m_remainingAllocationSize at every allocation (for
> > performance).
> > > Maybe we can stop the performance optimization (i.e., update the remaining
> > > allocation size at every allocation) in ASSERT builds and add asserts to
> > verify
> > > that m_remainingAllocationSize is holding a correct value.
> > 
> > I thought it was the prohibitive cost of doing the atomic operation of
> updating
> > the heap allocation counts for every call to allocate() that brought about
the
> > m_lastRemainingAllocationSize scheme --
> > 
> >  https://codereview.chromium.org/638223003/ +
> > https://codereview.chromium.org/634243004/
> 
> Yes, so my suggestion is to do the atomic operation only in ASSERT builds and
> add asserts to verify that m_lastRemainingAllocationSize is keeping track of a
> correct value.

Why do you only want to call Heap::increaseAllocatedObjectSize() only in ASSERT
builds? That's the atomic operation I'm referring to, perhaps we're talking
about separate matters here.

[haraken, 2015-11-01 19:55:39]

On 2015/11/01 19:48:58, sof wrote:
> On 2015/11/01 19:44:20, haraken wrote:
> > On 2015/11/01 19:35:10, sof wrote:
> > > On 2015/11/01 17:45:43, haraken wrote:
> > > > On 2015/11/01 12:18:50, sof wrote:
> > > > > please take a look.
> > > > > 
> > > > > Going after flaky asserts like,
> > > > > 
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa...
> > > > > 
> > > > > I think the problem is of a longer standing, but made more likely to
> > surface
> > > > > following https://codereview.chromium.org/1393863004
> > > > 
> > > > LGTM, thanks for fixing the flakiness!
> > > > 
> > > > The complexity around m_remainingAllocationSize come from the fact that
we
> > > don't
> > > > want to update m_remainingAllocationSize at every allocation (for
> > > performance).
> > > > Maybe we can stop the performance optimization (i.e., update the
remaining
> > > > allocation size at every allocation) in ASSERT builds and add asserts to
> > > verify
> > > > that m_remainingAllocationSize is holding a correct value.
> > > 
> > > I thought it was the prohibitive cost of doing the atomic operation of
> > updating
> > > the heap allocation counts for every call to allocate() that brought about
> the
> > > m_lastRemainingAllocationSize scheme --
> > > 
> > >  https://codereview.chromium.org/638223003/ +
> > > https://codereview.chromium.org/634243004/
> > 
> > Yes, so my suggestion is to do the atomic operation only in ASSERT builds
and
> > add asserts to verify that m_lastRemainingAllocationSize is keeping track of
a
> > correct value.
> 
> Why do you only want to call Heap::increaseAllocatedObjectSize() only in
ASSERT
> builds? That's the atomic operation I'm referring to, perhaps we're talking
> about separate matters here.

- Introduce s_allocatedObjectSizeUpdatedEveryAllocation only in ASSERT builds.
Update the size at every allocation.

- Add ASSERT(s_allocatedObjectSizeUpdatedEveryAllocation ==
s_allocateObjectSize) to places where m_lastRemainingAllocationSize is adjusted
to remainingAllocationSize(). If m_lastRemainingAllocationSize fails at keeping
track of a correct value, the ASSERT will hit at some point.

[sof, 2015-11-01 19:57:34]

Description was changed from

==========
Sync remaining allocation size on in-place backing store recycling.

When a heap's current allocation point can simply be moved after the tail
object allocation is either freed, expanded or shrunk, make sure the
snapshot'ed m_lastRemainingAllocationSize is then always adjusted at the
same time.

Not doing so risks later re-syncs of m_lastRemaininingAllocationSize to
be invoked on a "bigger" heap than last time, i.e.,
m_lastRemainingAllocationSize < m_remainingAllocationSize, which is an
unexpected state to be in. Simply avoid that state from ever being entered,
by synchronously updating m_lastRemainingAllocationSize.

setRemainingAllocationSize(size_t) handles this, along with updating the
overall heap allocation counts.

R=
BUG=420515
==========

to

==========
Sync remaining allocation size on in-place backing store recycling.

When a heap's current allocation point can simply be moved after the tail
object allocation is either freed, expanded or shrunk, make sure the
snapshot'ed m_lastRemainingAllocationSize is then always adjusted at the
same time.

Not doing so risks later re-syncs of m_lastRemaininingAllocationSize to
be invoked on a "bigger" heap than last time, i.e.,
m_lastRemainingAllocationSize < m_remainingAllocationSize, which is an
unexpected state to be in. Simply avoid that state from ever being entered,
by synchronously updating m_lastRemainingAllocationSize.

setRemainingAllocationSize(size_t) handles this, along with updating the
overall heap allocation counts.

R=haraken
BUG=420515
==========

[sof, 2015-11-01 19:57:45]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-01 19:58:00]

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

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-01 21:09:59]

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