Handle properties of builtin function separately from builtin objects #3694
Conversation
|
Would it be possible to create a |
|
@zherczeg |
|
It is just a question. If we create a const array, it will goes to the read-only data, so it increase the binary size, but not the read/write memory consumption. Since each function has a unique built-in id, we can get the name of the function from this const array. However, if no built-in routine use more than 16 properties, your patch could be good as well. |
|
Hi @clover2123! I'm planning to start the implementation of the function name property of script functions. To eliminate simultaneous work on this task, please let me know if you have already started it. :) |
|
@zherczeg AFAIK every name of built-in routines could be represented by 16bit lit_magic_string_id_t. @rerobika I'm going to implement 'name' property one by one starting from builtin function to normal function. But my work process would be of course slower than yours (for name of normal function, parser needs to store function name during the parsing. But I'm not familiar with jerryscript parser, so I have to dig into it now). |
|
This patch is still work-in-progress. |
|
@clover2123 yes, the name can be represented as a 16 bit integer. My question is, whether the instantiation of the actual properties (length, etc.) can be fit into 16 bit for all of these routines? If yes, then splitting the 32 bit value into two 16 bit values is ok. |
| getter_p = ecma_builtin_make_function_object_for_getter_accessor (builtin_id, getter_id); | ||
| setter_p = ecma_builtin_make_function_object_for_setter_accessor (builtin_id, setter_id); | ||
| getter_p = ecma_builtin_make_function_object_for_getter_accessor (builtin_id, | ||
| getter_id, curr_property_p->magic_string_id); |
There was a problem hiding this comment.
These arguments should go into separate lines.
| getter_p = ecma_builtin_make_function_object_for_getter_accessor (builtin_id, | ||
| getter_id, curr_property_p->magic_string_id); | ||
| setter_p = ecma_builtin_make_function_object_for_setter_accessor (builtin_id, | ||
| setter_id, curr_property_p->magic_string_id); |
| } | ||
| else | ||
| { | ||
| ecma_builtin_list_lazy_property_names (obj_p, |
There was a problem hiding this comment.
What about keeping this single function, and do this check at the beginnig of the function:
if (type == ECMA_OBJECT_TYPE_FUNCTION && ecma_builtin_function_is_routine (obj_p))
There was a problem hiding this comment.
IMHO builtin routine is a function property of builtin object (e.g. Math.abs), so it should be handled independently from builtin objects.
So, I added ecma_builtin_routine_list_lazy_property_names for builtin routines to clearly separate each builtin routine from builtin object.
This separation also follows other use case in which ecma_op_function_list_lazy_property_names and ecma_op_function_try_to_lazy_instantiate_property exist for function object.
jerry-core/ecma/base/ecma-globals.h
Outdated
| { | ||
| uint16_t name; /**< name of the built-in functions */ | ||
| uint16_t bitset; /**< bit set for instantiated properties of builtin functions */ | ||
| } builtin_function; |
There was a problem hiding this comment.
I think the name is misleading. This struct is specialized for routines, not for built-in functions (e.g Date).
|
Hi @clover2123! I'm interested about the status of the PR, since there was no activity in the last two weeks. The patch itself look good to me after the minor changes requested by @zherczeg , and we would like to land it soon. Do you have capacity to do the requested changes? If you don't, I can do the finals steps as well if you don't mind. |
|
@rerobika |
clover2123
force-pushed
the
name_property
branch
2 times, most recently
from
8b3aced to
ebdd8d3
Compare
jerry-core/ecma/base/ecma-globals.h
Outdated
| uint16_t id; /**< id of built-in routine */ | ||
| uint16_t name; /**< name of built-in routine */ | ||
| uint16_t bitset; /**< bit set for instantiated properties of builtin routine */ | ||
| } ecma_built_in_routine_props_t; |
There was a problem hiding this comment.
I am not sure what was the discussion here, but ecma_builtin_function_is_routine silently expects that id is mapped to the same position as routine_id. I usually prefer unions, since the common fileds are ensured to be in the same position. What do you think?
There was a problem hiding this comment.
It's my mistake.
Fixed it as you commented. Thanks:)
clover2123
force-pushed
the
name_property
branch
2 times, most recently
from
21cf2d1 to
e7b9d30
Compare
separate builtin routine part to make code clear and to support 'name' property later JerryScript-DCO-1.0-Signed-off-by: HyukWoo Park [email protected]
|
@clover2123 We are planning to prepare for a release before long and the function name support for builtins/builtin routines are highly important for us. I think this PR was only a preparation for it and according to your previous contributions you are interested about it. Would you like to submit a PR for finalize this topic or let us/me to do the remaining work? |
|
@rerobika |
|
@clover2123 We are planning in feature freeze in the end of this week please consider your available time for this task according to it and please let me know your decision. :) |
|
@rerobika |
4 participants
separate builtin function part to make code clear and to support 'name' property later
JerryScript-DCO-1.0-Signed-off-by: HyukWoo Park [email protected]