Issue 474913004: - Account for number of pending tasks in old-space collections.

Issue 474913004: - Account for number of pending tasks in old-space collections. (Closed)

Created:
6 years, 4 months ago by Ivan Posva

Modified:
6 years, 4 months ago

Reviewers:
koda, siva

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

- Account for number of pending tasks in old-space collections. - Protect access to the free lists. - MutexLocker/MonitorLocker do not need to be StackResources. R=koda@google.com Committed: https://code.google.com/p/dart/source/detail?r=39396

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Total comments: 11

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+292 lines, -106 lines)			Patch
M	runtime/lib/isolate.cc	View		4 chunks	+8 lines, -7 lines	0 comments	Download
M	runtime/vm/dart_api_impl_test.cc	View		3 chunks	+8 lines, -9 lines	0 comments	Download
M	runtime/vm/freelist.h	View		3 chunks	+10 lines, -2 lines	0 comments	Download
M	runtime/vm/freelist.cc	View	1	6 chunks	+21 lines, -3 lines	0 comments	Download
M	runtime/vm/heap.h	View	1	2 chunks	+3 lines, -22 lines	2 comments	Download
M	runtime/vm/isolate.cc	View		4 chunks	+4 lines, -10 lines	1 comment	Download
M	runtime/vm/lockers.h	View		2 chunks	+4 lines, -8 lines	0 comments	Download
M	runtime/vm/pages.h	View	1	5 chunks	+33 lines, -3 lines	0 comments	Download
M	runtime/vm/pages.cc	View		9 chunks	+51 lines, -16 lines	4 comments	Download
M	runtime/vm/parser.cc	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	runtime/vm/scavenger.cc	View		7 chunks	+8 lines, -3 lines	0 comments	Download
M	runtime/vm/snapshot.h	View		4 chunks	+6 lines, -1 line	2 comments	Download
M	runtime/vm/snapshot.cc	View		4 chunks	+31 lines, -11 lines	0 comments	Download
M	runtime/vm/thread.h	View		2 chunks	+9 lines, -0 lines	0 comments	Download
M	runtime/vm/thread_android.cc	View		5 chunks	+25 lines, -3 lines	2 comments	Download
M	runtime/vm/thread_linux.cc	View		5 chunks	+24 lines, -3 lines	0 comments	Download
M	runtime/vm/thread_macos.cc	View		5 chunks	+24 lines, -3 lines	0 comments	Download
M	runtime/vm/thread_win.cc	View		5 chunks	+22 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

koda

lgtm https://codereview.chromium.org/474913004/diff/1/runtime/vm/freelist.cc File runtime/vm/freelist.cc (right): https://codereview.chromium.org/474913004/diff/1/runtime/vm/freelist.cc#newcode182 runtime/vm/freelist.cc:182: void FreeList::FreeLocked(uword addr, intptr_t size) { Assert that ...

6 years, 4 months ago (2014-08-20 00:10:22 UTC) #2

Ivan Posva

Thanks, -Ivan https://codereview.chromium.org/474913004/diff/1/runtime/vm/freelist.cc File runtime/vm/freelist.cc (right): https://codereview.chromium.org/474913004/diff/1/runtime/vm/freelist.cc#newcode182 runtime/vm/freelist.cc:182: void FreeList::FreeLocked(uword addr, intptr_t size) { On ...

6 years, 4 months ago (2014-08-20 03:50:54 UTC) #3

Ivan Posva

Committed patchset #2 manually as r39396 (presubmit successful).

6 years, 4 months ago (2014-08-20 03:54:23 UTC) #4

siva

DBC. https://codereview.chromium.org/474913004/diff/20001/runtime/vm/heap.h File runtime/vm/heap.h (right): https://codereview.chromium.org/474913004/diff/20001/runtime/vm/heap.h#newcode66 runtime/vm/heap.h:66: PageSpace* old_space() { return old_space_; } const functions ...

6 years, 4 months ago (2014-08-20 17:21:23 UTC) #5

Ivan Posva

6 years, 4 months ago (2014-08-20 18:35:20 UTC) #6

Message was sent while issue was closed.

Thanks,
-Ivan

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

https://codereview.chromium.org/474913004/diff/20001/runtime/vm/heap.h#newcode66
runtime/vm/heap.h:66: PageSpace* old_space() { return old_space_; }
On 2014/08/20 17:21:22, siva wrote:
> const functions ?

Done.

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

https://codereview.chromium.org/474913004/diff/20001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:142: ASSERT(tasks() == 0);
On 2014/08/20 17:21:22, siva wrote:
> seems weird that a monitor locker is being done just for an assert,
> maybe the code should be
> #if defined(DEBUG)
> {
>   MonitorLocker ml(tasks_lock());
>   ASSERT(tasks() == 0);
> }
> #endif

  {
    MonitorLocker ml(tasks_lock());
    while(tasks() > 0) {
      ml.Wait();
    }
  }

https://codereview.chromium.org/474913004/diff/20001/runtime/vm/pages.cc#newc...
runtime/vm/pages.cc:631: MonitorLocker locker(tasks_lock());
On 2014/08/20 17:21:22, siva wrote:
> MutexLocker is sufficient here (no wait or notify is being done).

MutexLocker does not work with Monitors.

  {
    MonitorLocker ml(tasks_lock());
    ASSERT(tasks() == 1);
    set_tasks(tasks() - 1);
    ml.Notify();
  }

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

https://codereview.chromium.org/474913004/diff/20001/runtime/vm/snapshot.h#ne...
runtime/vm/snapshot.h:338: PageSpace* old_space() { return old_space_; }
On 2014/08/20 17:21:23, siva wrote:
> const function ?

Done.

https://codereview.chromium.org/474913004/diff/20001/runtime/vm/thread_androi...
File runtime/vm/thread_android.cc (right):

https://codereview.chromium.org/474913004/diff/20001/runtime/vm/thread_androi...
runtime/vm/thread_android.cc:199: ASSERT(owner_ == Isolate::Current());
On 2014/08/20 17:21:23, siva wrote:
> How does this assert work?
> owner_ field has not been initialized yet right?

Done. Thanks!

Expand Messages | Collapse Messages