Skip to content

Introducing getters and setters for compressed pointers in 'mem' component #57

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

Merged
merged 1 commit into from
May 13, 2015
Merged

Introducing getters and setters for compressed pointers in 'mem' component #57

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
4 changes: 2 additions & 2 deletions jerry-core/ecma/base/ecma-globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@
* The offset is shifted right by MEM_ALIGNMENT_LOG.
* Least significant MEM_ALIGNMENT_LOG bits of non-shifted offset are zeroes.
*/
#define ECMA_POINTER_FIELD_WIDTH MEM_COMPRESSED_POINTER_WIDTH
#define ECMA_POINTER_FIELD_WIDTH MEM_CP_WIDTH

/**
* The NULL value for compressed pointers
*/
#define ECMA_NULL_POINTER MEM_COMPRESSED_POINTER_NULL
#define ECMA_NULL_POINTER MEM_CP_NULL

/**
* @}
Expand Down
35 changes: 9 additions & 26 deletions jerry-core/ecma/base/ecma-helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,44 +27,27 @@
#include "mem-allocator.h"

/**
* Get value of pointer from specified non-null compressed pointer field.
* Get value of pointer from specified non-null compressed pointer.
*/
#define ECMA_GET_NON_NULL_POINTER(type, field) \
((type *) mem_decompress_pointer (field))
#define ECMA_GET_NON_NULL_POINTER(type, field) MEM_CP_GET_NON_NULL_POINTER (type, field)

/**
* Get value of pointer from specified compressed pointer field.
* Get value of pointer from specified compressed pointer.
*/
#define ECMA_GET_POINTER(type, field) \
(((unlikely (field == ECMA_NULL_POINTER)) ? NULL : ECMA_GET_NON_NULL_POINTER (type, field)))
#define ECMA_GET_POINTER(type, field) MEM_CP_GET_POINTER (type, field)

/**
* Set value of non-null compressed pointer field so that it will correspond
* Set value of non-null compressed pointer so that it will correspond
* to specified non_compressed_pointer.
*/
#define ECMA_SET_NON_NULL_POINTER(field, non_compressed_pointer) \
(field) = (mem_compress_pointer (non_compressed_pointer) & \
((((mem_cpointer_t) 1u) << ECMA_POINTER_FIELD_WIDTH) - 1))
#define ECMA_SET_NON_NULL_POINTER(field, non_compressed_pointer) MEM_CP_SET_NON_NULL_POINTER (field, \
non_compressed_pointer)

/**
* Set value of compressed pointer field so that it will correspond
* Set value of compressed pointer so that it will correspond
* to specified non_compressed_pointer.
*/
#define ECMA_SET_POINTER(field, non_compressed_pointer) \
do \
{ \
auto __temp_pointer = non_compressed_pointer; \
non_compressed_pointer = __temp_pointer; \
} while (0); \
\
if (unlikely ((non_compressed_pointer) == NULL)) \
{ \
(field) = ECMA_NULL_POINTER; \
} \
else \
{ \
ECMA_SET_NON_NULL_POINTER (field, non_compressed_pointer); \
}
#define ECMA_SET_POINTER(field, non_compressed_pointer) MEM_CP_SET_POINTER (field, non_compressed_pointer)

/* ecma-helpers-value.c */
extern bool ecma_is_value_empty (ecma_value_t value);
Expand Down
4 changes: 2 additions & 2 deletions jerry-core/mem/mem-allocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ mem_compress_pointer (const void *pointer) /**< pointer to compress */

JERRY_ASSERT((int_ptr & ~((1u << MEM_HEAP_OFFSET_LOG) - 1)) == 0);

JERRY_ASSERT(int_ptr != MEM_COMPRESSED_POINTER_NULL);
JERRY_ASSERT (int_ptr != MEM_CP_NULL);

return int_ptr;
} /* mem_compress_pointer */
Expand All @@ -120,7 +120,7 @@ mem_compress_pointer (const void *pointer) /**< pointer to compress */
void*
mem_decompress_pointer (uintptr_t compressed_pointer) /**< pointer to decompress */
{
JERRY_ASSERT(compressed_pointer != MEM_COMPRESSED_POINTER_NULL);
JERRY_ASSERT (compressed_pointer != MEM_CP_NULL);

uintptr_t int_ptr = compressed_pointer;

Expand Down
45 changes: 42 additions & 3 deletions jerry-core/mem/mem-allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ typedef uint16_t mem_cpointer_t;
/**
* Representation of NULL value for compressed pointers
*/
#define MEM_COMPRESSED_POINTER_NULL 0
#define MEM_CP_NULL 0

/**
* Required alignment for allocated units/blocks
Expand All @@ -46,12 +46,12 @@ typedef uint16_t mem_cpointer_t;
/**
* Width of compressed memory pointer
*/
#define MEM_COMPRESSED_POINTER_WIDTH (MEM_HEAP_OFFSET_LOG - MEM_ALIGNMENT_LOG)
#define MEM_CP_WIDTH (MEM_HEAP_OFFSET_LOG - MEM_ALIGNMENT_LOG)

/**
* Compressed pointer value mask
*/
#define MEM_COMPRESSED_POINTER_MASK ((1ull << MEM_COMPRESSED_POINTER_WIDTH) - 1)
#define MEM_CP_MASK ((1ull << MEM_CP_WIDTH) - 1)

/**
* Heap offset value mask
Expand Down Expand Up @@ -80,6 +80,45 @@ typedef enum
*/
typedef void (*mem_try_give_memory_back_callback_t) (mem_try_give_memory_back_severity_t);

/**
* Get value of pointer from specified non-null compressed pointer value
*/
#define MEM_CP_GET_NON_NULL_POINTER(type, cp_value) \
((type *) mem_decompress_pointer (cp_value))

/**
* Get value of pointer from specified compressed pointer value
*/
#define MEM_CP_GET_POINTER(type, cp_value) \
(((unlikely ((cp_value) == MEM_CP_NULL)) ? NULL : MEM_CP_GET_NON_NULL_POINTER (type, cp_value)))

/**
* Set value of non-null compressed pointer so that it will correspond
* to specified non_compressed_pointer
*/
#define MEM_CP_SET_NON_NULL_POINTER(cp_value, non_compressed_pointer) \
(cp_value) = (mem_compress_pointer (non_compressed_pointer) & MEM_CP_MASK)

/**
* Set value of compressed pointer so that it will correspond
* to specified non_compressed_pointer
*/
#define MEM_CP_SET_POINTER(cp_value, non_compressed_pointer) \
do \
{ \
auto __temp_pointer = non_compressed_pointer; \
non_compressed_pointer = __temp_pointer; \
} while (0); \
\
if (unlikely ((non_compressed_pointer) == NULL)) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider the following example usage:

 if (something_should_be_true(..))
    MEM_CP_SET_POINTER(...);
else
   ....

that example would lead to a compile error, and if you leave out the else case then it will compile, but won't do what you really wanted.

The macro should have been written in the following format, to avoid such problems:

#define MEM_CP_SET_POINTER(cp_value, non_compressed_pointer) \
do \
{ \
  auto ....  \
  ....  \
  if ... \
  else ... \
} while (0)

Note that there is no semicolon at the end of the while (0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, Jerryscript's style requires braces on every control construction.

So, the if would be written as the following:

 if (something_should_be_true(..))
 {
    MEM_CP_SET_POINTER(...);
 }
 else
 {
   ....
 }

And, the example would expand to:

 if (something_should_be_true(..))
 {
   do
   {
     auto __temp_pointer = non_compressed_pointer;
     non_compressed_pointer = __temp_pointer;
   } while (0);

   if (unlikely ((non_compressed_pointer) == NULL))
   {
     (cp_value) = MEM_CP_NULL;
   }
   else
   {
     MEM_CP_SET_NON_NULL_POINTER (cp_value, non_compressed_pointer);
   }
 }
 else
 {
   ....
 }

Anyway, thanks for the advice.
I agree that your implementation of the macro is more secure and should have been used in the case.

{ \
(cp_value) = MEM_CP_NULL; \
} \
else \
{ \
MEM_CP_SET_NON_NULL_POINTER (cp_value, non_compressed_pointer); \
}

extern void mem_init (void);
extern void mem_finalize (bool is_show_mem_stats);

Expand Down
2 changes: 1 addition & 1 deletion jerry-core/mem/mem-pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ typedef struct __attribute__ ((aligned (MEM_ALIGNMENT))) mem_pool_state_t
mem_pool_chunk_index_t free_chunks_number : MEM_POOL_MAX_CHUNKS_NUMBER_LOG;

/** Pointer to the next pool with same chunk size */
mem_cpointer_t next_pool_cp : MEM_COMPRESSED_POINTER_WIDTH;
mem_cpointer_t next_pool_cp : MEM_CP_WIDTH;
} mem_pool_state_t;

extern void mem_pool_init (mem_pool_state_t *pool_p, size_t pool_size);
Expand Down
30 changes: 6 additions & 24 deletions jerry-core/mem/mem-poolman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,7 @@ mem_pools_alloc_longpath (void)

mem_pool_init (pool_state, MEM_POOL_SIZE);

if (mem_pools == NULL)
{
pool_state->next_pool_cp = MEM_COMPRESSED_POINTER_NULL;
}
else
{
pool_state->next_pool_cp = mem_compress_pointer (mem_pools) & MEM_COMPRESSED_POINTER_MASK;
}
MEM_CP_SET_POINTER (pool_state->next_pool_cp, mem_pools);

mem_pools = pool_state;

Expand All @@ -137,15 +130,13 @@ mem_pools_alloc_longpath (void)
while (pool_state->first_free_chunk == MEM_POOL_CHUNKS_NUMBER)
{
prev_pool_state_p = pool_state;
pool_state = (mem_pool_state_t*) mem_decompress_pointer (pool_state->next_pool_cp);

JERRY_ASSERT(pool_state != NULL);
pool_state = MEM_CP_GET_NON_NULL_POINTER (mem_pool_state_t, pool_state->next_pool_cp);
}

JERRY_ASSERT (prev_pool_state_p != NULL && pool_state != mem_pools);

prev_pool_state_p->next_pool_cp = pool_state->next_pool_cp;
pool_state->next_pool_cp = mem_compress_pointer (mem_pools) & MEM_COMPRESSED_POINTER_MASK;
MEM_CP_SET_NON_NULL_POINTER (pool_state->next_pool_cp, mem_pools);
mem_pools = pool_state;
}

Expand Down Expand Up @@ -195,9 +186,7 @@ mem_pools_free (uint8_t *chunk_p) /**< pointer to the chunk */
while (!mem_pool_is_chunk_inside (pool_state, chunk_p))
{
prev_pool_state_p = pool_state;
pool_state = (mem_pool_state_t*) mem_decompress_pointer (pool_state->next_pool_cp);

JERRY_ASSERT(pool_state != NULL);
pool_state = MEM_CP_GET_NON_NULL_POINTER (mem_pool_state_t, pool_state->next_pool_cp);
}

/**
Expand All @@ -219,14 +208,7 @@ mem_pools_free (uint8_t *chunk_p) /**< pointer to the chunk */
}
else
{
if (pool_state->next_pool_cp == MEM_COMPRESSED_POINTER_NULL)
{
mem_pools = NULL;
}
else
{
mem_pools = (mem_pool_state_t*) mem_decompress_pointer (pool_state->next_pool_cp);
}
mem_pools = MEM_CP_GET_POINTER (mem_pool_state_t, pool_state->next_pool_cp);
}

mem_free_chunks_number -= MEM_POOL_CHUNKS_NUMBER;
Expand All @@ -240,7 +222,7 @@ mem_pools_free (uint8_t *chunk_p) /**< pointer to the chunk */
JERRY_ASSERT (prev_pool_state_p != NULL);

prev_pool_state_p->next_pool_cp = pool_state->next_pool_cp;
pool_state->next_pool_cp = mem_compress_pointer (mem_pools) & MEM_COMPRESSED_POINTER_MASK;
MEM_CP_SET_NON_NULL_POINTER (pool_state->next_pool_cp, mem_pools);
mem_pools = pool_state;
}
} /* mem_pools_free */
Expand Down