Skip to content

Don't use pointer compression where the ecma_value can store pointer … #1020

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.

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

Conversation

robertsipka
Copy link
Contributor

…value directly.

This changes affects those internal properties where the property value type is an ecma_object_t ptr.

JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com

@robertsipka robertsipka force-pushed the internal_property branch 4 times, most recently from d6b1156 to 019d92b Compare April 20, 2016 22:08
@LaszloLango LaszloLango added enhancement An improvement ecma builtins Related to ECMA built-in routines labels Apr 21, 2016
@LaszloLango
Copy link
Contributor

LaszloLango commented Apr 21, 2016

LGTM

@@ -315,7 +315,8 @@ ecma_gc_mark_property (ecma_property_t *property_p) /**< property */
case ECMA_INTERNAL_PROPERTY_SCOPE: /* a lexical environment */
case ECMA_INTERNAL_PROPERTY_PARAMETERS_MAP: /* an object */
{
ecma_object_t *obj_p = ECMA_GET_NON_NULL_POINTER (ecma_object_t, property_value);

ecma_object_t *obj_p = ecma_get_object_from_value (property_value);
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this is still a slow-down, since you also add object flags to the value. I would still prefer a variant to the ECMA_GET_NON_NULL_POINTER macro.

ECMA_INTERNAL_VALUE_GET_POINTER / ECMA_INTERNAL_VALUE_SET_POINTER

@robertsipka robertsipka force-pushed the internal_property branch 3 times, most recently from 2ce61c1 to e84dad6 Compare April 22, 2016 09:14
@robertsipka
Copy link
Contributor Author

Thanks, I've updated the patch based on your comment.

* Set an internal property value of non-null pointer so that it will correspond
* to specified non_compressed_pointer.
*/
#define ECMA_SET_INTERNAL_VALUE_POINTER(field, non_compressed_pointer) ECMA_SET_NON_NULL_POINTER(field, \
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer

#define ECMA_SET_INTERNAL_VALUE_POINTER(field, non_compressed_pointer) \
  ECMA_SET_NON_NULL_POINTER((field), (non_compressed_pointer))

@robertsipka robertsipka force-pushed the internal_property branch 2 times, most recently from d1e0e3b to 8cf7c44 Compare April 22, 2016 10:02
@robertsipka
Copy link
Contributor Author

I've rebase with master.

/**
* Set an internal property value of pointer
*/
#define ECMA_SET_INTERNAL_VALUE_POINTER(field, pointer) field = (ecma_value_t) pointer;
Copy link
Contributor

@LaszloLango LaszloLango Apr 22, 2016

Choose a reason for hiding this comment

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

I'd use similar style for the macro like below.

#define ECMA_SET_INTERNAL_VALUE_POINTER(field, non_compressed_pointer) \
  field = (ecma_value_t) non_compressed_pointer

@robertsipka robertsipka force-pushed the internal_property branch 3 times, most recently from 963e951 to f3a942b Compare April 22, 2016 11:50
…value directly.

This changes affects those internal properties where the property value type is an ecma_object_t ptr.

JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
@zherczeg
Copy link
Member

LGTM

@LaszloLango
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecma builtins Related to ECMA built-in routines enhancement An improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants