Reimplemented zone memory tracking to avoid race conditions that were causing crashes in the previo…

Reimplemented zone memory tracking to avoid race conditions that were causing crashes in the previous implementation and made some minor name changes.

BUG=
R=asiva@google.com, johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/c0511ad2ef920518513b986e16735bf9c158aeb8

[bkonyi, 2017-03-21 20:41:14]

bkonyi@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2017-03-21 20:51:57]

lgtm

[bkonyi, 2017-03-22 17:14:45]

With Realloc double-count fix.

[Cutch, 2017-03-22 17:21:50]

Final round of comments.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.cc
File runtime/vm/zone.cc (right):

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:50: current_thread->DecrementMemoryCapacity(current->size());
ditto here:

IncrementThreadMemoryCapacity / DecrementThreadMemoryCapacity

Fold all the current_thread != NULL checks into those two functions and call
those functions instead of littering the code with the checks.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:215: if (current_thread != NULL) {
Refactor this code so there are two helper functions:

IncrementThreadMemoryUsage(size) {
Thread* current_thread = Thread::Current();
if (current_thread == null) { return; }
current_thread->IncrementMemoryUsage(size);
}

Same for DecrementMemoryUsage

Use these functions instead of sprinkling all these (current_thread != NULL)
checks all over.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:249: Thread* current_thread = Thread::Current();
move this Thread::Current() call so it only happens if you're going to use
current_thread.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:261: if (current_thread != NULL && previous != position_) {
Wrap each of these != expressions in parens.

[siva, 2017-03-22 17:50:55]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2017-03-22 17:50:55]

DBC

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (right):

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:334: }
How did the current_thread get into the threads array?

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:214: Thread* current_thread = Thread::Current();
This is an overhead on every zone allocation path, have you measured the impact
of doing this on the VM performance?

Is it critical to measure the actual memory usage Vs the current capacity that
has been allocated? We have already allocated that memory from the system how
does it matter whether we used it or not as far as our footprint goes that
memory has been allocated.

[bkonyi, 2017-03-22 18:01:48]

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (right):

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:334: }
On 2017/03/22 17:50:55, siva wrote:
> How did the current_thread get into the threads array?

Good catch. I guess when I originally wrote this test I was also checking the
state of the current thread. Since we need to skip it now, I'll just remove the
current thread from the threads array.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.cc
File runtime/vm/zone.cc (right):

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:50: current_thread->DecrementMemoryCapacity(current->size());
On 2017/03/22 17:21:49, Cutch wrote:
> ditto here:
> 
> IncrementThreadMemoryCapacity / DecrementThreadMemoryCapacity
> 
> Fold all the current_thread != NULL checks into those two functions and call
> those functions instead of littering the code with the checks.

Done.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h
File runtime/vm/zone.h (right):

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:214: Thread* current_thread = Thread::Current();
On 2017/03/22 17:50:55, siva wrote:
> This is an overhead on every zone allocation path, have you measured the
impact
> of doing this on the VM performance?
> 
> Is it critical to measure the actual memory usage Vs the current capacity that
> has been allocated? We have already allocated that memory from the system how
> does it matter whether we used it or not as far as our footprint goes that
> memory has been allocated.

No, I haven't run a performance test yet. Now that you mention it, I agree that
the zone usage statistic isn't very useful compared to the zone capacity value,
unless we're concerned that there's potential issues in how/when the zones
decide to expand / handle reallocations. Thoughts?

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:215: if (current_thread != NULL) {
On 2017/03/22 17:21:49, Cutch wrote:
> Refactor this code so there are two helper functions:
> 
> IncrementThreadMemoryUsage(size) {
> Thread* current_thread = Thread::Current();
> if (current_thread == null) { return; }
> current_thread->IncrementMemoryUsage(size);
> }
> 
> Same for DecrementMemoryUsage
> 
> Use these functions instead of sprinkling all these (current_thread != NULL)
> checks all over.

Done.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:249: Thread* current_thread = Thread::Current();
On 2017/03/22 17:21:50, Cutch wrote:
> move this Thread::Current() call so it only happens if you're going to use
> current_thread.

Done.

https://codereview.chromium.org/2762323002/diff/20001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:261: if (current_thread != NULL && previous != position_) {
On 2017/03/22 17:21:50, Cutch wrote:
> Wrap each of these != expressions in parens.

Acknowledged.

[bkonyi, 2017-03-22 22:15:07]

I've gone ahead and removed the zone usage information. I think everything
should be good to go now, unless anyone has more comments.

[siva, 2017-03-22 23:36:06]

lgtm

https://codereview.chromium.org/2762323002/diff/60001/runtime/vm/zone.cc
File runtime/vm/zone.cc (right):

https://codereview.chromium.org/2762323002/diff/60001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:34: static void DecrementMemoryCapacity(uintptr_t size);
The two versions of IncrementMemoryCapacity/DecrementMemoryCapacity here and in
class Zone could be shared.

[Cutch, 2017-03-23 13:47:47]

lgtm

[bkonyi, 2017-03-23 16:03:14]

Description was changed from

==========
Reimplemented zone memory tracking to avoid race conditions that were causing
crashes in the previous implementation and made some minor name changes.

BUG=
==========

to

==========
Reimplemented zone memory tracking to avoid race conditions that were causing
crashes in the previous implementation and made some minor name changes.

BUG=
R=asiva@google.com, johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c0511ad2ef920518513b986e16735bf9c158aeb8
==========

[bkonyi, 2017-03-23 16:03:15]

Committed patchset #5 (id:80001) manually as
c0511ad2ef920518513b986e16735bf9c158aeb8 (presubmit successful).