Skip to content

Remove trivial ecma_number arithmetic functions #2123

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

rerobika
Copy link
Member

@rerobika rerobika commented Nov 24, 2017

The affected function calls have been replaced with the appropriate arithmetic operands.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]

@LaszloLango
Copy link
Contributor

I don't see any improvement here. This is just a noise. I think these inlining are done by compiler automatically, because the binary size did not increase.

@rerobika
Copy link
Member Author

@LaszloLango I've updated the result with the run time standard deviation and highlighted the tests and results where the gain is magnitude greater then the std deviation. That's why I think it's more then noise.

@rerobika
Copy link
Member Author

Also I've compared the two binaries (with patch and without patch) and there is difference between them. It's means inlining these functions has real effect.

@@ -687,7 +687,7 @@ ecma_number_calc_remainder (ecma_number_t left_num, /**< left operand */
*
* @return number - result of addition.
*/
ecma_number_t
inline ecma_number_t __attr_always_inline___
Copy link
Member

Choose a reason for hiding this comment

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

Please just remove these functions.

@rerobika rerobika force-pushed the inline_ecma_operations branch from e8153f2 to b4bcc77 Compare November 30, 2017 16:47
@rerobika rerobika changed the title Decrease run time by inlining short functions Remove trivial ecma_number arithmetic functions Nov 30, 2017
@rerobika
Copy link
Member Author

@zherczeg Your suggestion have been applied!

@rerobika rerobika force-pushed the inline_ecma_operations branch from b4bcc77 to 4d3bbef Compare November 30, 2017 16:49
ecma_number_t new_value = ecma_number_add (ecma_get_float_from_value (left_value),
ecma_get_number_from_value (right_value));
ecma_number_t new_value = ecma_get_float_from_value (left_value) +
ecma_get_number_from_value (right_value);
Copy link
Member

Choose a reason for hiding this comment

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

This is not nice alignment. What is the difference between float value and number value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Number value not specifies that the stored value is float or integer. Unfortunately it is not a nice alignment, but this is what vera++ requires.

Copy link
Member

Choose a reason for hiding this comment

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

I think you can use () brackets to help aligning

Copy link
Member Author

Choose a reason for hiding this comment

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

It worked! Thanks for your advice!

The affected function calls have been replaced with the appropriate arithmetic operands.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]
@rerobika rerobika force-pushed the inline_ecma_operations branch from 4d3bbef to f1fcbf8 Compare December 4, 2017 09:46
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

@LaszloLango LaszloLango merged commit 90f2473 into jerryscript-project:master Dec 7, 2017
@rerobika rerobika deleted the inline_ecma_operations branch April 16, 2018 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants