New External Magic String API to save heap memory

[seanshpark]

Related issue: #109

JerryScript-DCO-1.0-Signed-off-by: SaeHie Park [email protected]

[ruben-ayrapetyan]

@seanshpark, could we implement the external magic strings with registration of a constant structures array, using a structure like the following?

typedef struct
{
  const jerry_char_t* value_p;
  jerry_api_length_t length;
} jerry_ext_magic_string_t;

This way, we can improve performance by removing call to an external handler while getting value of an external magic string.

[seanshpark]

yes, sure.

[seanshpark]

with an jerry_ext_magic_string_t array I think ecma_magic_string_ex_handler is not needed any more. it can be accessed something like

ecma_magic_string_ex_table[id].value_p;

[ruben-ayrapetyan]

@seanshpark, do I understand correctly that the external magic strings mechanism is supposed to be used for registration of externally defined identifiers like console, log, etc.?

[seanshpark]

@ruben-ayrapetyan , yes, you are right. reason is to save heap for byte codes.

[ruben-ayrapetyan]

@seanshpark, thank you for explanation. That's great!

[seanshpark]

@ruben-ayrapetyan , as memory consumption point of view, this implementation requires NUM_OF_STRINGS * sizeof(ecma_length_t = uint16_t) bytes for ecma_magic_string_ex_lengths array. when changed to structure it needs additional NUM_OF_STRINGS * sizeof(const jerry_char_t*) and as usually structure is aligned in 4 bytes, it'll be 4 times the size. for example, if there is about 256 items, it'll need 512 bytes to store lengths, and for structure 2048 (= 256 * (4+4)) bytes. when measure with last week iotjs JS script it saved about 1.8K for about 180 string items in total.(measured peak with mem-stats on)
so I'm not sure about changing to structure, though I prefer the structure type.
please let me know if I missed something.

[seanshpark]

@ruben-ayrapetyan , updated PR so that string array is used without the handler but not by structure you mentioned.

[ruben-ayrapetyan]

@seanshpark, I agree with you, that alignment of structure increases memory consumption and, so, using structure for defining external strings seems not to be the right way.

Could you, please, change the jerry_register_external_magic_strings interface to take array of lengths, that already contains strings' lengths, and add debug mode check that it is filled correctly?

Maybe, the lengths' array could be defined at compile-time similar to strings' array definition using sizeof operator.

In the case, we could make the strings' and lengths' arrays constant, would remove filling of externally defined array inside of engine, and, maybe, would remove runtime lengths' calculation.

[seanshpark]

@ruben-ayrapetyan , thank you, PR is updated with your new idea. now no heap is used (I think). great. maybe we can change existing magic string like this one.

[ruben-ayrapetyan]

Could you, please, add comments to the arguments?

/**< character arrays, representing
  *    external magic strings' contents */
/**< number of the strings */
/**< lengths of the strings */

[seanshpark]

Done as @ruben-ayrapetyan' comment.
afd532d fails cause __program needs to be NULL. 2b2f2df is to solve this.

[ruben-ayrapetyan]

Please, change the definitions using uint8_t and uint16_t with single definition using ecma_char_t.

[ruben-ayrapetyan]

@seanshpark, thank you for updating the changes.

Could you please, reorder commits in the pull request, to fix build problem?

After fixing build and API type definitions, the changes would look good for me.

[egavrin]

As for me, all tree commits should go with separate pull requests.

@seanshpark after passing the review, please, apply these commits to master separately.

[seanshpark]

@ruben-ayrapetyan ,

1. last three comments

Please, change the definitions using uint8_t and uint16_t with single definition using ecma_char_t.
Please, change uint16_t to ecma_length_t.
Please, change jerry_api_char_t* to ecma_char_ptr_t.

cannot apply. compile error. ecma_globals.h is not included and I'm not sure it can be.

2. re-ordered commits to fix build error

@egavrin ,
ok, three separate commits as in this pull request.

[ruben-ayrapetyan]

@seanshpark, thank you for update. Changes look good to me.

Sorry for incorrect comment about type definitions.
I think, later the type definitions would be moved to another header that would be common to the whole engine and API, so the definitions would not be duplicated.

[egavrin]

Great patch! make push