Reviewers: Vyacheslav Egorov,
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc
File src/spaces.cc (left):
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#oldcode2625
src/spaces.cc:2625:
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
accidental edit? please revert.
Done.
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc
File src/spaces.cc (right):
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2523
src/spaces.cc:2523: static bool match(void* key1, void* key2) {
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
Please give this function a more meaningful name, e.g.
ComparePointers/SamePointers/etc
Done.
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2526
src/spaces.cc:2526:
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
one additional empty line
Done.
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2588
src/spaces.cc:2588: // map following entries to this page object, in
which keys are
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
comments should start with a capital letter and end with a dot.
Done.
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2595
src/spaces.cc:2595: key += MemoryChunk::kAlignment, hash++) {
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
I think you can just unify hash and key (make key == hash) because key
is
aligned and it's 20 less significant bits are always zeros. This will
make this
loop cleaner.
Done.
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2596
src/spaces.cc:2596: (map_.Lookup(reinterpret_cast<void*>(key), hash,
true))->value = page;
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
I would prefer this is done in two lines instead of a single line.
Done.
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2624
src/spaces.cc:2624: if ( page_address <= a && a < page_address +
page->size() ) {
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
accidental edit? please revert.
Done.
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2642
src/spaces.cc:2642: return page;
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
This code is duplicated in two places. Please move everything to a
single helper
function.
Accepted, will reduce FindObject(a) size by calling this function
instead to remove duplicated code
http://codereview.chromium.org/9634005/diff/1/src/spaces.cc#newcode2679
src/spaces.cc:2679: // remove entries belonging to this page
On 2012/03/08 11:41:23, Vyacheslav Egorov wrote:
comments should start with a capital letter and end with a dot.
Done.
http://codereview.chromium.org/9634005/diff/7002/src/spaces.cc
File src/spaces.cc (right):
http://codereview.chromium.org/9634005/diff/7002/src/spaces.cc#newcode2591
src/spaces.cc:2591: uint32_t base =
reinterpret_cast<uint32_t>(page)/MemoryChunk::kAlignment;
On 2012/03/09 12:15:37, Vyacheslav Egorov wrote:
casting page to uint32_t is not suitable for 64-bit arch.
consider using uintptr_t instead.
Done.
http://codereview.chromium.org/9634005/diff/7002/src/spaces.cc#newcode2616
src/spaces.cc:2616: LargePage* page = FindPageContainingPc(a);
On 2012/03/09 12:15:37, Vyacheslav Egorov wrote:
I would suggest retaining FindPageContainingPc because you are calling
it from
the function that is used for arbitrary addresses.
Not very sure if I catch your point. Please allow me to confirm. Do you
suggest retaining the linear search of FindPageContainingPc() and just
change FindObject() to hash lookup, or reversed, or something else ?
Thanks a lot for your answering
http://codereview.chromium.org/9634005/diff/7002/src/spaces.cc#newcode2633
src/spaces.cc:2633: if (page_address <= pc && pc < page_address +
page->size()) {
On 2012/03/09 12:15:37, Vyacheslav Egorov wrote:
Consider using page->Contains(pc) here.
The check range of page->Contains(pc) falls into
(base+Page::kObjectStartOffset, base+chunk_size), so if pc falls into
page's header range (base, base+Page::kObjectStartOffset) will be
failed, which would succeed otherwise. Should we allow this difference
and replace it with Contains() anyway ?
http://codereview.chromium.org/9634005/diff/7002/src/spaces.cc#newcode2673
src/spaces.cc:2673: uint32_t base =
reinterpret_cast<uint32_t>(page)/MemoryChunk::kAlignment;
On 2012/03/09 12:15:37, Vyacheslav Egorov wrote:
casting page to uint32_t is not suitable for 64-bit arch.
consider using uintptr_t instead.
Done.
Description:
Implement a hash based look-up to speed up containing address check in large
object space. Before, it was a link-list based look-up, and make this
function
a little bit 'hot' from profile point.
BUG=v8:853
TEST=
Please review this at http://codereview.chromium.org/9634005/
SVN Base: http://v8.googlecode.com/svn/trunk/
Affected files:
M src/spaces.h
M src/spaces.cc
Index: src/spaces.cc
===================================================================
--- src/spaces.cc (revision 10961)
+++ src/spaces.cc (working copy)
@@ -2520,7 +2520,11 @@
//
-----------------------------------------------------------------------------
// LargeObjectSpace
+static bool ComparePointers(void* key1, void* key2) {
+ return key1 == key2;
+}
+
LargeObjectSpace::LargeObjectSpace(Heap* heap,
intptr_t max_capacity,
AllocationSpace id)
@@ -2529,7 +2533,8 @@
first_page_(NULL),
size_(0),
page_count_(0),
- objects_size_(0) {}
+ objects_size_(0),
+ chunk_map_(ComparePointers, 1024) {}
bool LargeObjectSpace::SetUp() {
@@ -2537,6 +2542,7 @@
size_ = 0;
page_count_ = 0;
objects_size_ = 0;
+ chunk_map_.Clear();
return true;
}
@@ -2580,6 +2586,17 @@
page->set_next_page(first_page_);
first_page_ = page;
+ // Register all MemoryChunk::kAlignment-aligned chunks covered by
+ // this large page in the chunk map.
+ uint32_t base = reinterpret_cast<uint32_t>(page)/MemoryChunk::kAlignment;
+ uint32_t limit = base + (page->size()-1)/MemoryChunk::kAlignment;
+ for (uint32_t key = base; key <= limit; key++) {
+ HashMap::Entry* entry = chunk_map_.Lookup(reinterpret_cast<void*>(key),
+ key, true);
+ ASSERT(entry != NULL);
+ entry->value = page;
+ }
+
HeapObject* object = page->GetObject();
#ifdef DEBUG
@@ -2596,27 +2613,25 @@
// GC support
MaybeObject* LargeObjectSpace::FindObject(Address a) {
- for (LargePage* page = first_page_;
- page != NULL;
- page = page->next_page()) {
- Address page_address = page->address();
- if (page_address <= a && a < page_address + page->size()) {
- return page->GetObject();
- }
+ LargePage* page = FindPageContainingPc(a);
+ if (page != NULL) {
+ return page->GetObject();
}
return Failure::Exception();
}
LargePage* LargeObjectSpace::FindPageContainingPc(Address pc) {
- // TODO(853): Change this implementation to only find executable
- // chunks and use some kind of hash-based approach to speed it up.
- for (LargePage* chunk = first_page_;
- chunk != NULL;
- chunk = chunk->next_page()) {
- Address chunk_address = chunk->address();
- if (chunk_address <= pc && pc < chunk_address + chunk->size()) {
- return chunk;
+ uint32_t key = reinterpret_cast<uint32_t>(pc) / MemoryChunk::kAlignment;
+ HashMap::Entry* e = chunk_map_.Lookup(reinterpret_cast<void*>(key),
+ key, false);
+ if (e != NULL) {
+ ASSERT(e->value != NULL);
+ LargePage* page = reinterpret_cast<LargePage*>(e->value);
+ ASSERT(page->is_valid());
+ Address page_address = page->address();
+ if (page_address <= pc && pc < page_address + page->size()) {
+ return page;
}
}
return NULL;
@@ -2654,6 +2669,13 @@
objects_size_ -= object->Size();
page_count_--;
+ // Remove entries belonging to this page.
+ uint32_t base =
reinterpret_cast<uint32_t>(page)/MemoryChunk::kAlignment;
+ uint32_t limit = base + (page->size()-1)/MemoryChunk::kAlignment;
+ for (uint32_t key = base; key <= limit; key++) {
+ chunk_map_.Remove(reinterpret_cast<void*>(key), key);
+ }
+
if (is_pointer_object) {
heap()->QueueMemoryChunkForFree(page);
} else {
Index: src/spaces.h
===================================================================
--- src/spaces.h (revision 10961)
+++ src/spaces.h (working copy)
@@ -29,6 +29,7 @@
#define V8_SPACES_H_
#include "allocation.h"
+#include "hashmap.h"
#include "list.h"
#include "log.h"
@@ -2536,6 +2537,8 @@
intptr_t size_; // allocated bytes
int page_count_; // number of chunks
intptr_t objects_size_; // size of objects
+ // Map MemoryChunk::kAlignment-aligned chunks to large pages covering
them
+ HashMap chunk_map_;
friend class LargeObjectIterator;
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
