Skip to content

Fast search of data space start for one-chunked blocks #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fast search of data space start for one-chunked blocks #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion jerry-core/mem/mem-allocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
/**
* Area for heap
*/
static uint8_t mem_heap_area[ MEM_HEAP_AREA_SIZE ] __attribute__ ((aligned (MEM_ALIGNMENT)));
static uint8_t mem_heap_area[ MEM_HEAP_AREA_SIZE ] __attribute__ ((aligned (JERRY_MAX (MEM_ALIGNMENT,
MEM_HEAP_CHUNK_SIZE))));

/**
* The 'try to give memory back' callback
Expand Down
68 changes: 43 additions & 25 deletions jerry-core/mem/mem-heap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,15 +396,22 @@ mem_is_block_free (const mem_block_header_t *block_header_p) /**< block header *

/**
* Startup initialization of heap
*
* Note:
* heap start and size should be aligned on MEM_HEAP_CHUNK_SIZE
*/
void
mem_heap_init (uint8_t *heap_start, /**< first address of heap space */
size_t heap_size) /**< heap space size */
{
JERRY_ASSERT (heap_start != NULL);
JERRY_ASSERT (heap_size != 0);
JERRY_ASSERT (heap_size % MEM_HEAP_CHUNK_SIZE == 0);

JERRY_STATIC_ASSERT ((MEM_HEAP_CHUNK_SIZE & (MEM_HEAP_CHUNK_SIZE - 1u)) == 0);
JERRY_ASSERT ((uintptr_t) heap_start % MEM_ALIGNMENT == 0);
JERRY_ASSERT ((uintptr_t) heap_start % MEM_HEAP_CHUNK_SIZE == 0);
JERRY_ASSERT (heap_size % MEM_HEAP_CHUNK_SIZE == 0);

JERRY_ASSERT (heap_size <= (1u << MEM_HEAP_OFFSET_LOG));

mem_heap.heap_start = heap_start;
Expand Down Expand Up @@ -800,6 +807,7 @@ mem_heap_free_block (void *ptr) /**< pointer to beginning of data space of the b

/* marking the block free */
block_p->allocated_bytes = 0;
block_p->length_type = mem_block_length_type_t::GENERAL;

if (next_block_p != NULL)
{
Expand Down Expand Up @@ -866,12 +874,12 @@ mem_heap_free_block (void *ptr) /**< pointer to beginning of data space of the b
} /* mem_heap_free_block */

/**
* Find beginning of user data in a block from pointer,
* Find beginning of user data in a one-chunked block from pointer,
* pointing into it, i.e. into [block_data_space_start; block_data_space_end) range.
*
* Note:
* Pointer must point to the memory region which was previously allocated
* with mem_heap_alloc_block and is currently valid.
* Pointer must point to the one-chunked memory region which was previously allocated
* with mem_heap_alloc_chunked_block and is currently valid.
*
* Note:
* The interface should only be used for determining where the user space of heap-allocated block begins.
Expand All @@ -880,49 +888,59 @@ mem_heap_free_block (void *ptr) /**< pointer to beginning of data space of the b
* @return beginning of user data space of block identified by the pointer
*/
void*
mem_heap_get_block_start (void *ptr) /**< pointer into a block */
mem_heap_get_chunked_block_start (void *ptr) /**< pointer into a block */
{
mem_check_heap ();

/*
* PERF: consider introducing bitmap of block beginnings
*/
JERRY_STATIC_ASSERT ((MEM_HEAP_CHUNK_SIZE & (MEM_HEAP_CHUNK_SIZE - 1u)) == 0);
JERRY_ASSERT (((uintptr_t) mem_heap.heap_start % MEM_HEAP_CHUNK_SIZE) == 0);

JERRY_ASSERT (mem_heap.heap_start <= ptr
&& ptr < mem_heap.heap_start + mem_heap.heap_size);

const mem_block_header_t *block_p = mem_heap.first_block_p;
uintptr_t uintptr = (uintptr_t) ptr;
uintptr_t uintptr_chunk_aligned = JERRY_ALIGNDOWN (uintptr, MEM_HEAP_CHUNK_SIZE);

JERRY_ASSERT (uintptr > uintptr_chunk_aligned);

mem_block_header_t *block_p = (mem_block_header_t *) uintptr_chunk_aligned;
JERRY_ASSERT (block_p->length_type == mem_block_length_type_t::ONE_CHUNKED);

#ifndef JERRY_NDEBUG
const mem_block_header_t *block_iter_p = mem_heap.first_block_p;
bool is_found = false;

/* searching for corresponding block */
while (block_p != NULL)
while (block_iter_p != NULL)
{
VALGRIND_DEFINED_STRUCT (block_p);
VALGRIND_DEFINED_STRUCT (block_iter_p);

const mem_block_header_t *next_block_p = mem_get_next_block_by_direction (block_p,
const mem_block_header_t *next_block_p = mem_get_next_block_by_direction (block_iter_p,
MEM_DIRECTION_NEXT);
bool is_found = (ptr > block_p
&& (ptr < next_block_p
|| next_block_p == NULL));
is_found = (ptr > block_iter_p
&& (ptr < next_block_p
|| next_block_p == NULL));

if (is_found)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this pull request, but I think these lines (922..934), can be revised:

JERRY_ASSERT (is_found && !mem_is_block_free (block_iter_p));
JERRY_ASSERT (is_found && block_iter_p + 1 <= ptr);
JERRY_ASSERT (is_found && ptr < ((uint8_t*) (block_iter_p + 1) + block_iter_p->allocated_bytes));

VALGRIND_NOACCESS_STRUCT (block_iter_p);

if (is_found)
{
   break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what view point?
Could you, please, describe in more details?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two separate if (is_found), as for me, this condition should be inside JERRY_ASSERT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
The logic here is "if block is found, assert the following its properties", so if (is_found) is more readable in the case. Considering that the whole loop containing the if is intended for debug (compiled under !JERRY_NDEBUG condition), this style seems to be suitable for the case.
Otherwise, we would write several lines similar to the following:
JERRY_ASSERT (!is_found || (condition));

{
JERRY_ASSERT (!mem_is_block_free (block_p));
JERRY_ASSERT (block_p + 1 <= ptr);
JERRY_ASSERT (ptr < ((uint8_t*) (block_p + 1) + block_p->allocated_bytes));
JERRY_ASSERT (!mem_is_block_free (block_iter_p));
JERRY_ASSERT (block_iter_p + 1 <= ptr);
JERRY_ASSERT (ptr < ((uint8_t*) (block_iter_p + 1) + block_iter_p->allocated_bytes));
}

VALGRIND_NOACCESS_STRUCT (block_p);
VALGRIND_NOACCESS_STRUCT (block_iter_p);

if (is_found)
{
return (void*) (block_p + 1);
break;
}

block_p = next_block_p;
block_iter_p = next_block_p;
}

JERRY_UNREACHABLE ();
} /* mem_heap_get_block_start */
JERRY_ASSERT (is_found && block_p == block_iter_p);
#endif /* !JERRY_NDEBUG */

return (void*) (block_p + 1);
} /* mem_heap_get_chunked_block_start */

/**
* Get size of one-chunked block data space
Expand Down
2 changes: 1 addition & 1 deletion jerry-core/mem/mem-heap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ extern void mem_heap_finalize (void);
extern void* mem_heap_alloc_block (size_t size_in_bytes, mem_heap_alloc_term_t alloc_term);
extern void* mem_heap_alloc_chunked_block (mem_heap_alloc_term_t alloc_term);
extern void mem_heap_free_block (void *ptr);
extern void* mem_heap_get_block_start (void *ptr);
extern void* mem_heap_get_chunked_block_start (void *ptr);
extern size_t mem_heap_get_chunked_block_data_size (void);
extern size_t __attr_pure___ mem_heap_recommend_allocation_size (size_t minimum_allocation_size);
extern void mem_heap_print (bool dump_block_headers, bool dump_block_data, bool dump_stats);
Expand Down
43 changes: 31 additions & 12 deletions tests/unit/test_heap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ extern "C"

uint8_t *ptrs[test_sub_iters];
size_t sizes[test_sub_iters];
bool is_one_chunked[test_sub_iters];

static void
test_heap_give_some_memory_back (mem_try_give_memory_back_severity_t severity)
Expand Down Expand Up @@ -82,12 +83,13 @@ test_heap_give_some_memory_back (mem_try_give_memory_back_severity_t severity)
}
} /* test_heap_give_some_memory_back */

uint8_t test_native_heap[test_heap_size] __attribute__ ((aligned (JERRY_MAX (MEM_ALIGNMENT,
MEM_HEAP_CHUNK_SIZE))));

int
main (int __attr_unused___ argc,
char __attr_unused___ **argv)
{
uint8_t test_native_heap[test_heap_size];

mem_heap_init (test_native_heap, sizeof (test_native_heap));

srand ((unsigned int) time (NULL));
Expand All @@ -103,17 +105,31 @@ main (int __attr_unused___ argc,
{
for (uint32_t j = 0; j < test_sub_iters; j++)
{
size_t size = (size_t) rand () % test_threshold_block_size;
ptrs[j] = (uint8_t*) mem_heap_alloc_block (size,
(rand () % 2) ?
MEM_HEAP_ALLOC_LONG_TERM : MEM_HEAP_ALLOC_SHORT_TERM);
sizes[j] = size;
if (rand () % 2)
{
size_t size = (size_t) rand () % test_threshold_block_size;
ptrs[j] = (uint8_t*) mem_heap_alloc_block (size,
(rand () % 2) ?
MEM_HEAP_ALLOC_LONG_TERM : MEM_HEAP_ALLOC_SHORT_TERM);
sizes[j] = size;
is_one_chunked[j] = false;
}
else
{
ptrs[j] = (uint8_t*) mem_heap_alloc_chunked_block ((rand () % 2) ?
MEM_HEAP_ALLOC_LONG_TERM : MEM_HEAP_ALLOC_SHORT_TERM);
sizes[j] = mem_heap_get_chunked_block_data_size ();
is_one_chunked[j] = true;
}

JERRY_ASSERT (size == 0 || ptrs[j] != NULL);
JERRY_ASSERT (sizes[j] == 0 || ptrs[j] != NULL);
memset (ptrs[j], 0, sizes[j]);

JERRY_ASSERT (ptrs[j] == NULL
|| mem_heap_get_block_start (ptrs[j] + (size_t) rand () % sizes[j]) == ptrs[j]);
if (is_one_chunked[j])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can merge this condition into assert? Same for below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could do it.
But, perhaps, it would be more readable without the merge, because so we see that assertion is intended explicitly for one-chunked blocks.

{
JERRY_ASSERT (ptrs[j] != NULL
&& mem_heap_get_chunked_block_start (ptrs[j] + (size_t) rand () % sizes[j]) == ptrs[j]);
}
}

// mem_heap_print (true);
Expand All @@ -127,8 +143,11 @@ main (int __attr_unused___ argc,
JERRY_ASSERT (ptrs[j][k] == 0);
}

JERRY_ASSERT (sizes[j] == 0
|| mem_heap_get_block_start (ptrs[j] + (size_t) rand () % sizes[j]) == ptrs[j]);
if (is_one_chunked[j])
{
JERRY_ASSERT (sizes[j] == 0
|| mem_heap_get_chunked_block_start (ptrs[j] + (size_t) rand () % sizes[j]) == ptrs[j]);
}

mem_heap_free_block (ptrs[j]);

Expand Down