- Consolidate code into the old generation.

- Consolidate code into the old generation.
- Record pointers between code objects.
- Collect unreferenced code objects.
Committed: https://code.google.com/p/dart/source/detail?r=14113

[srdjan, 2012-10-25 20:00:48]

DBC

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc
File vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:762: intptr_t len = resolved_static_calls.Length();
const intptr_t len

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:764: for (intptr_t i = 0; i < len; i += 2) {
Maybe it would be good to have the length, entries of the resolved static calls
array specified in an enum.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:778: resolved_static_calls.SetAt(index + 1, target_code);
ditto

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:795: Code& target_code =
Code::Handle(target_function.CurrentCode());
const Code

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:800: Code& code =
Code::Handle(caller_frame->LookupDartCode());
ditto

[siva, 2012-10-25 22:29:52]

lgtm

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc
File vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:803: target_code);
bool found = Update....;
ASSERT(!found);

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc
File vm/heap.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc#newcode133
vm/heap.cc:133: RawInstructions* Heap::FindObjectInOldSpace(FindObjectVisitor*
visitor) {
The signature and name of the function does not seem to jive.
It should either be
RawObject* Heap::FindObjectInOldSpace(...);
meaning find any object based on the visitor function

or

RawInstructions* Heap::FindInstructionObject(..... visitor);
which would imply it finds specific instruction objects in the heap.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc#newcode134
vm/heap.cc:134: // The code heap can only have RawInstructions objects.
This comment doesn't jive anymore...

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc#newcode137
vm/heap.cc:137: (raw_obj->GetClassId() == kInstructionsCid));
If we choose to go with the generic find object functionality then this ASSERT
here is incorrect.

I would go with generic find object and move this assertion and cast etc. to
Code::LookupCode

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.cc
File vm/pages.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.cc#newcode422
vm/pages.cc:422: // Reset the freelists and setup sweeping.
It is probably more generic to write this as:
for (intptr_t i = 0; i < HeapPage::kNumPageTypes; i++) {
  freelist_[i].Reset();
}

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.cc#newcode483
vm/pages.cc:483: freelist_[HeapPage::kExecutable].Print();
Ditto comment:
It is probably more generic to write this as:
for (intptr_t i = 0; i < HeapPage::kNumPageTypes; i++) {
  OS::Print("Type %d:\n". i);
  freelist_[i].Print();
}

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.h
File vm/pages.h (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.h#newcode79
vm/pages.h:79: bool executable_;
should this just be
PageType type_;

[Ivan Posva, 2012-10-25 23:45:41]

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc
File vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:762: intptr_t len = resolved_static_calls.Length();
On 2012/10/25 20:00:48, srdjan wrote:
> const intptr_t len

Done.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:764: for (intptr_t i = 0; i < len; i += 2) {
On 2012/10/25 20:00:48, srdjan wrote:
> Maybe it would be good to have the length, entries of the resolved static
calls
> array specified in an enum.

Done.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:778: resolved_static_calls.SetAt(index + 1, target_code);
On 2012/10/25 20:00:48, srdjan wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:795: Code& target_code =
Code::Handle(target_function.CurrentCode());
On 2012/10/25 20:00:48, srdjan wrote:
> const Code

Done.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:800: Code& code =
Code::Handle(caller_frame->LookupDartCode());
On 2012/10/25 20:00:48, srdjan wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:803: target_code);
On 2012/10/25 22:29:52, siva wrote:
> bool found = Update....;
> ASSERT(!found);

Done.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc
File vm/heap.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc#newcode133
vm/heap.cc:133: RawInstructions* Heap::FindObjectInOldSpace(FindObjectVisitor*
visitor) {
On 2012/10/25 22:29:52, siva wrote:
> The signature and name of the function does not seem to jive.
> It should either be
> RawObject* Heap::FindObjectInOldSpace(...);
> meaning find any object based on the visitor function
> 
> or
> 
> RawInstructions* Heap::FindInstructionObject(..... visitor);
> which would imply it finds specific instruction objects in the heap.
I'll stick with this second option.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc#newcode134
vm/heap.cc:134: // The code heap can only have RawInstructions objects.
On 2012/10/25 22:29:52, siva wrote:
> This comment doesn't jive anymore...

Done.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/heap.cc#newcode137
vm/heap.cc:137: (raw_obj->GetClassId() == kInstructionsCid));
On 2012/10/25 22:29:52, siva wrote:
> If we choose to go with the generic find object functionality then this ASSERT
> here is incorrect.
> 
> I would go with generic find object and move this assertion and cast etc. to
> Code::LookupCode
Went the other way...

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.cc
File vm/pages.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.cc#newcode422
vm/pages.cc:422: // Reset the freelists and setup sweeping.
On 2012/10/25 22:29:52, siva wrote:
> It is probably more generic to write this as:
> for (intptr_t i = 0; i < HeapPage::kNumPageTypes; i++) {
>   freelist_[i].Reset();
> }

Yes, once we have more page types.

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.cc#newcode483
vm/pages.cc:483: freelist_[HeapPage::kExecutable].Print();
On 2012/10/25 22:29:52, siva wrote:
> Ditto comment:
> It is probably more generic to write this as:
> for (intptr_t i = 0; i < HeapPage::kNumPageTypes; i++) {
>   OS::Print("Type %d:\n". i);
>   freelist_[i].Print();
> }

ditto

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.h
File vm/pages.h (right):

https://chromiumcodereview.appspot.com/11265026/diff/1/vm/pages.h#newcode79
vm/pages.h:79: bool executable_;
On 2012/10/25 22:29:52, siva wrote:
> should this just be
> PageType type_;

If it really is needed, then I will switch this to a bit field. Leaving it as is
for now.

[cshapiro, 2012-10-26 04:58:07]

https://chromiumcodereview.appspot.com/11265026/diff/1014/vm/gc_sweeper.cc
File vm/gc_sweeper.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1014/vm/gc_sweeper.cc#ne...
vm/gc_sweeper.cc:56: memset(reinterpret_cast<void*>(current), 0xcc, obj_size);
Can we put this behind an compile time flag, like what is done in the scavenger?

#if defined(DEBUG)
      if (is_executable) {
        memset(reinterpret_cast<void*>(current), 0xcc, obj_size);
      }
#endif  // DEBUG

[Ivan Posva, 2012-10-26 05:00:45]

https://chromiumcodereview.appspot.com/11265026/diff/1014/vm/gc_sweeper.cc
File vm/gc_sweeper.cc (right):

https://chromiumcodereview.appspot.com/11265026/diff/1014/vm/gc_sweeper.cc#ne...
vm/gc_sweeper.cc:56: memset(reinterpret_cast<void*>(current), 0xcc, obj_size);
On 2012/10/26 04:58:07, cshapiro wrote:
> Can we put this behind an compile time flag, like what is done in the
scavenger?
> 
> #if defined(DEBUG)
>       if (is_executable) {
>         memset(reinterpret_cast<void*>(current), 0xcc, obj_size);
>       }
> #endif  // DEBUG
>  

I left this specifically in for now while this is new to ensure that we do not
end up jumping into free'd code. I will add a TODO to move this under DEBUG
though.

[cshapiro, 2012-10-26 05:06:55]

Sounds reasonable, thanks.

You might also want to add a TODO for picking a target specific constant.  0xCC
works for x86 but a different opcode is needed for ARM.