Skip to content

Various enhancements #914

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

Jump to bottom
Merged
merged 1 commit into from
Mar 1, 2016
Merged

Various enhancements #914

merged 1 commit into from
Mar 1, 2016

Conversation

bzsolt
Copy link
Member

@bzsolt bzsolt commented Feb 23, 2016

Modifications:

  • eliminate unnecessary variables, functions
  • use ECMA_NUMBER macros where it is possible
  • simplify code
  • minor style fix (comments, increase-decrease operators)

JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély zsborbely.u-szeged@partner.samsung.com

@bzsolt bzsolt force-pushed the random_enhancements branch from b5a0a5c to 4c87f0a Compare February 23, 2016 09:33
@bzsolt bzsolt changed the title Random enhancements Various enhancements Feb 23, 2016
@bzsolt
Copy link
Member Author

bzsolt commented Feb 23, 2016

RPi2 perf results
master (9d79780)

                   Benchmark |                  Perf(+ is better)
                  3d-cube.js |  2.637s ->   2.618s :  +0.696%  (+-0.887%) : [≈]
      access-binary-trees.js |  1.410s ->   1.408s :  +0.118%  (+-0.477%) : [≈]
          access-fannkuch.js |  7.982s ->   7.973s :  +0.104%  (+-0.584%) : [≈]
             access-nbody.js |  3.083s ->   3.152s :  -2.216%  (+-0.596%) : [-]
 bitops-3bit-bits-in-byte.js |  1.733s ->   1.727s :  +0.384%  (+-0.692%) : [≈]
      bitops-bits-in-byte.js |  2.655s ->   2.657s :  -0.063%  (+-0.467%) : [≈]
       bitops-bitwise-and.js |  3.683s ->   3.738s :  -1.493%  (+-0.703%) : [-]
    controlflow-recursive.js |  0.890s ->   0.893s :  -0.374%  (+-0.955%) : [≈]
               crypto-aes.js |  4.720s ->   4.668s :  +1.095%  (+-0.675%) : [+]
               crypto-md5.js | 21.537s ->  21.497s :  +0.186%  (+-0.505%) : [≈]
              crypto-sha1.js |  9.725s ->   9.743s :  -0.188%  (+-0.825%) : [≈]
        date-format-tofte.js |  2.230s ->   2.210s :  +0.897%  (+-1.138%) : [≈]
        date-format-xparb.js |  1.080s ->   1.073s :  +0.618%  (+-0.787%) : [≈]
              math-cordic.js |  2.993s ->   3.000s :  -0.223%  (+-0.666%) : [≈]
        math-partial-sums.js |  1.665s ->   1.677s :  -0.701%  (+-0.747%) : [≈]
       math-spectral-norm.js |  1.513s ->   1.495s :  +1.211%  (+-1.061%) : [+]
             string-fasta.js |  4.555s ->   4.530s :  +0.549%  (+-1.263%) : [≈]
             Geometric mean: |                Speed up: 0.039% (+-0.194%) : [≈]

akosthekiss
akosthekiss reviewed Feb 23, 2016
View reviewed changes
jerry-core/ecma/base/ecma-helpers-conversion.c
@@ -818,12 +818,10 @@ ecma_uint32_to_utf8_string (uint32_t value, /**< value to convert */
*
* @return number - result of conversion.
*/
ecma_number_t
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.

Is this needed? Where does it make a difference? Performance results show no significant change that would justify the need for this attribute. (BTW, does it have an effect on the code size?)

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 doesn't make any difference. The binary sizes are the same.
There are the results without this attributes:
(It's possible, that this gain is only noise)

                  Benchmark |                   Perf(+ is better)
                 3d-cube.js |   2.645s ->   2.617s :  +1.071%  (+-0.824%) : [+]
     access-binary-trees.js |   1.410s ->   1.430s :  -1.418%  (+-0.000%) : [-]
         access-fannkuch.js |   7.985s ->   7.997s :  -0.146%  (+-0.460%) : [≈]
            access-nbody.js |   3.083s ->   3.115s :  -1.027%  (+-0.526%) : [-]
bitops-3bit-bits-in-byte.js |   1.737s ->   1.713s :  +1.344%  (+-0.688%) : [+]
     bitops-bits-in-byte.js |   2.660s ->   2.647s :  +0.501%  (+-0.929%) : [≈]
      bitops-bitwise-and.js |   3.682s ->   3.780s :  -2.671%  (+-1.079%) : [-]
   controlflow-recursive.js |   0.890s ->   0.887s :  +0.374%  (+-0.955%) : [≈]
              crypto-aes.js |   4.727s ->   4.695s :  +0.670%  (+-0.512%) : [+]
              crypto-md5.js |  21.542s ->  21.543s :  -0.007%  (+-0.451%) : [≈]
             crypto-sha1.js |   9.767s ->   9.760s :  +0.068%  (+-0.448%) : [≈]
       date-format-tofte.js |   2.233s ->   2.217s :  +0.746%  (+-0.846%) : [≈]
       date-format-xparb.js |   1.085s ->   1.077s :  +0.768%  (+-1.137%) : [≈]
             math-cordic.js |   3.005s ->   2.980s :  +0.832%  (+-0.829%) : [+]
       math-partial-sums.js |   1.665s ->   1.680s :  -0.901%  (+-0.546%) : [-]
      math-spectral-norm.js |   1.513s ->   1.487s :  +1.762%  (+-0.787%) : [+]
            string-fasta.js |   4.542s ->   4.557s :  -0.330%  (+-0.611%) : [≈]
            Geometric mean: |                 Speed up: 0.102% (+-0.179%) : [≈]

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't add the attributes then. IMHO, the cleaner the code the better (i.e., we should avoid using attributes as much as we can).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @akiss77. If it doesn't have any performance gain, then don't add it.

@akosthekiss
Copy link
Member

And in general, the file headers should be double-checked. The headers of some now-touched files bear a 2014 copyright.

@bzsolt bzsolt force-pushed the random_enhancements branch from 4c87f0a to 692adb2 Compare February 23, 2016 13:14
@bzsolt
Copy link
Member Author

bzsolt commented Feb 23, 2016

@akiss77 @LaszloLango The PR is updated according the review.

@akosthekiss
Copy link
Member

Thanks. LGTM (FWIW)

@LaszloLango LaszloLango added enhancement An improvement style Related to coding style labels Feb 23, 2016
@LaszloLango
Copy link
Contributor

It is strange that 'ecma_is_ex_string_magic' is not used in Jerry. May be removing it not the best solution. Isn't this feature needed for IoT.js?

@bzsolt
Copy link
Member Author

bzsolt commented Feb 23, 2016

I didn't find any reference to this function in IoT.js, but there's a chance it will be used in the future, I agree with you, we should keep it. So I will update the PR.

@LaszloLango
Copy link
Contributor

@bzsolt, could you raise an issue for this?

LaszloLango
LaszloLango reviewed Feb 23, 2016
View reviewed changes
jerry-core/ecma/base/ecma-helpers-string.c
const bool is_equal_hashes = (string1_p->hash == string2_p->hash);

if (!is_equal_hashes)
if (!(string1_p->hash == string2_p->hash))
Copy link
Contributor

Choose a reason for hiding this comment

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

if (string1_p->hash != string2_p->hash)

@LaszloLango
Copy link
Contributor

After fix of #914 (comment) the patch LGTM.

@bzsolt bzsolt force-pushed the random_enhancements branch from 692adb2 to b1854cb Compare February 25, 2016 14:13
@bzsolt
Copy link
Member Author

bzsolt commented Feb 25, 2016

@LaszloLango I've updated the PR. I've kept the external magic string related functions, and raised an issue for this topic: #922

@zherczeg
Copy link
Member

I would call the patch "small refactorings".
LGTM

@bzsolt bzsolt force-pushed the random_enhancements branch 2 times, most recently from cc8708d to 7c3b68e Compare March 1, 2016 08:05
Small refactorings
3f37769 
Modifications:
* eliminate unnecessary variables, functions
* use ECMA_NUMBER macros where it is possible
* simplify code
* minor style fix (comments, increase-decrease operators)

JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély zsborbely.u-szeged@partner.samsung.com
@bzsolt bzsolt force-pushed the random_enhancements branch from 7c3b68e to 3f37769 Compare March 1, 2016 12:21
@LaszloLango LaszloLango merged commit 3f37769 into jerryscript-project:master Mar 1, 2016
@zherczeg zherczeg mentioned this pull request Apr 7, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement An improvement style Related to coding style
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bzsolt @akosthekiss @LaszloLango @zherczeg