Skip to content

Remove unnecessary variable length arrays. #851

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
Feb 10, 2016
Merged

Remove unnecessary variable length arrays. #851

merged 1 commit into from
Feb 10, 2016

Conversation

robertsipka
Copy link
Contributor

VLAs cannot be initialized by any form of initialization syntax with C99 standard.

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

@LaszloLango LaszloLango added the enhancement An improvement label Feb 8, 2016
@akosthekiss
Copy link
Member

The ECMA_STRING_CONTAINER_UINT32_IN_DESC paths in ecma_string_get_size and ecma_string_get_length are very close clones of each other. The first one has no VLA actually.

What about factoring the clones out into a helper function and using:

const uint32_t nums_with_ascending_length[] = { ... };
const uint32_t max_uint32_len = sizeof(nums_with_ascending_length) / sizeof(uint32_t);

This would avoid multiplication.

BTW, while we are at the refactoring of these functions: the switch/case in ecma_string_get_size looks much better to me than the if/else if/else if/else chain in ecma_string_get_length.

BTW2, the implementation of the ECMA_STRING_CONTAINER_HEAP_NUMBER paths is also suspiciously similar in the two functions. Candidates for yet another helper?

@robertsipka robertsipka force-pushed the remove_variable_length_arrays branch from e8c60b5 to 1f0ed97 Compare February 8, 2016 15:22
@robertsipka
Copy link
Contributor Author

Thanks the comments, I've updated this PR.

@robertsipka robertsipka force-pushed the remove_variable_length_arrays branch from 1f0ed97 to c2b3be4 Compare February 8, 2016 15:28
akosthekiss
akosthekiss reviewed Feb 8, 2016
View reviewed changes
jerry-core/ecma/base/ecma-helpers-string.cpp
@@ -1286,6 +1286,42 @@ ecma_compare_ecma_strings_relational (const ecma_string_t *string1_p, /**< ecma-
} /* ecma_compare_ecma_strings_relational */

/**
* Lengths for numeric string values
*/
const uint32_t nums_with_ascending_length[] =
Copy link
Member

Choose a reason for hiding this comment

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

Should be static

@robertsipka robertsipka force-pushed the remove_variable_length_arrays branch from c2b3be4 to 7daabb3 Compare February 9, 2016 08:35
@robertsipka
Copy link
Contributor Author

Thanks again, I've fixed this based on your suggestions.

akosthekiss
akosthekiss reviewed Feb 9, 2016
View reviewed changes
jerry-core/ecma/base/ecma-helpers-string.cpp
* @return size in bytes
*/
static ecma_length_t
ecma_string_get_number_in_desc_size (const uint32_t uint32_number)
Copy link
Member

Choose a reason for hiding this comment

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

Please add comment on argument.
moreover, I'd swap the order of these two new helper functions to get this one closer to the data it uses (nums_with_ascending_length and max_uint32_len).

@robertsipka robertsipka force-pushed the remove_variable_length_arrays branch from 7daabb3 to 0faaaa8 Compare February 9, 2016 09:20
@robertsipka
Copy link
Contributor Author

Fixed.

@akosthekiss
Copy link
Member

LGTM (informally)

@LaszloLango
Copy link
Contributor

@robertsipka, Could you measure this patch on RPi2 or at least on your own PC? @bzsolt, can you help with the measurement on RPi2? We have to be careful when refactoring these functions, because they are called really often.

@bzsolt
Copy link
Member

bzsolt commented Feb 9, 2016

The RPi2 results with run-perf-test.sh
master: 91a0514

Benchmark RSS
(+ is better)		 Perf
(+ is better)
3d-cube.js   96k ->   92k      
             +4.167%		   2.700s ->   2.700s                      
                 +0.000%  (+-0.862%) : [≈]
access-binary-trees.js   84k ->   84k      
             +0.000%		   1.490s ->   1.505s                      
                 -1.007%  (+-1.167%) : [≈]
access-fannkuch.js   40k ->   40k      
             +0.000%		   8.035s ->   8.177s                      
                 -1.763%  (+-0.805%) : [-]
access-nbody.js   48k ->   44k      
             +8.333%		   3.230s ->   3.357s                      
                 -3.922%  (+-0.962%) : [-]
bitops-3bit-bits-in-byte.js   32k ->   32k      
             +0.000%		   1.762s ->   1.797s                      
                 -1.987%  (+-1.054%) : [-]
bitops-bits-in-byte.js   32k ->   32k      
             +0.000%		   2.690s ->   2.712s                      
                 -0.806%  (+-0.818%) : [≈]
bitops-bitwise-and.js   32k ->   32k      
             +0.000%		   3.728s ->   3.805s                      
                 -2.056%  (+-0.646%) : [-]
controlflow-recursive.js  152k ->  148k      
             +2.632%		   0.900s ->   0.920s                      
                 -2.222%  (+-0.000%) : [-]
crypto-aes.js  124k ->  124k      
             +0.000%		   4.752s ->   4.792s                      
                 -0.842%  (+-1.097%) : [≈]
crypto-md5.js  184k ->  184k      
             +0.000%		  21.153s ->  21.138s                      
                 +0.071%  (+-0.748%) : [≈]
crypto-sha1.js  132k ->  128k      
             +3.030%		   9.582s ->   9.637s                      
                 -0.574%  (+-0.714%) : [≈]
date-format-tofte.js   64k ->   64k      
             +0.000%		   2.225s ->   2.233s                      
                 -0.374%  (+-0.558%) : [≈]
date-format-xparb.js   64k ->   64k      
             +0.000%		   1.145s ->   1.145s                      
                 +0.000%  (+-1.114%) : [≈]
math-cordic.js   32k ->   32k      
             +0.000%		   3.040s ->   3.150s                      
                 -3.618%  (+-0.783%) : [-]
math-partial-sums.js   36k ->   36k      
             +0.000%		   1.853s ->   1.910s                      
                 -3.058%  (+-1.082%) : [-]
math-spectral-norm.js   44k ->   44k      
             +0.000%		   1.535s ->   1.547s                      
                 -0.760%  (+-0.811%) : [≈]
string-fasta.js   48k ->   48k      
             +0.000%		   4.607s ->   4.620s                      
                 -0.289%  (+-0.584%) : [≈]
Geometric mean: RSS reduction: 1.094% Speed up: -1.358% (+-0.205%) : [-]

@LaszloLango
Copy link
Contributor

Could you try how the performance change if you make the ecma_string_get_number_in_desc_size and ecma_string_get_heap_number_size inline with __attr_always_inline___?

@LaszloLango LaszloLango self-assigned this Feb 9, 2016
@bzsolt
Copy link
Member

bzsolt commented Feb 9, 2016

Of course, the results are here:

Benchmark RSS
(+ is better)		 Perf
(+ is better)
3d-cube.js   96k ->   96k      
             +0.000%		   2.702s ->   2.710s                      
                 -0.308%  (+-0.898%) : [≈]
access-binary-trees.js   84k ->   84k      
             +0.000%		   1.487s ->   1.495s                      
                 -0.560%  (+-0.836%) : [≈]
access-fannkuch.js   40k ->   40k      
             +0.000%		   8.035s ->   8.187s                      
                 -1.888%  (+-0.560%) : [-]
access-nbody.js   48k ->   44k      
             +8.333%		   3.238s ->   3.270s                      
                 -0.978%  (+-1.133%) : [≈]
bitops-3bit-bits-in-byte.js   32k ->   32k      
             +0.000%		   1.755s ->   1.750s                      
                 +0.285%  (+-0.783%) : [≈]
bitops-bits-in-byte.js   32k ->   32k      
             +0.000%		   2.690s ->   2.677s                      
                 +0.495%  (+-0.738%) : [≈]
bitops-bitwise-and.js   32k ->   32k      
             +0.000%		   3.713s ->   3.823s                      
                 -2.962%  (+-0.590%) : [-]
controlflow-recursive.js  152k ->  148k      
             +2.632%		   0.898s ->   0.905s                      
                 -0.742%  (+-1.255%) : [≈]
crypto-aes.js  124k ->  124k      
             +0.000%		   4.753s ->   4.765s                      
                 -0.246%  (+-0.673%) : [≈]
crypto-md5.js  184k ->  184k      
             +0.000%		  21.153s ->  21.085s                      
                 +0.323%  (+-0.799%) : [≈]
crypto-sha1.js  132k ->  128k      
             +3.030%		   9.585s ->   9.588s                      
                 -0.035%  (+-0.960%) : [≈]
date-format-tofte.js   64k ->   64k      
             +0.000%		   2.230s ->   2.237s                      
                 -0.299%  (+-1.294%) : [≈]
date-format-xparb.js   64k ->   64k      
             +0.000%		   1.143s ->   1.145s                      
                 -0.146%  (+-1.416%) : [≈]
math-cordic.js   32k ->   32k      
             +0.000%		   3.037s ->   3.033s                      
                 +0.110%  (+-0.791%) : [≈]
math-partial-sums.js   36k ->   36k      
             +0.000%		   1.853s ->   1.863s                      
                 -0.540%  (+-1.027%) : [≈]
math-spectral-norm.js   44k ->   44k      
             +0.000%		   1.537s ->   1.522s                      
                 +0.976%  (+-0.701%) : [+]
string-fasta.js   48k ->   48k      
             +0.000%		   4.618s ->   4.625s                      
                 -0.144%  (+-0.787%) : [≈]
Geometric mean: RSS reduction: 0.846% Speed up: -0.388% (+-0.225%) : [-]

@LaszloLango
Copy link
Contributor

@bzsolt, looks better.
@robertsipka, please update the PR.

@robertsipka robertsipka force-pushed the remove_variable_length_arrays branch from 0faaaa8 to fbcda55 Compare February 10, 2016 08:39
@robertsipka
Copy link
Contributor Author

@bzsolt @LaszloLango : thanks, fixed.

@LaszloLango
Copy link
Contributor

LGTM

@LaszloLango
Copy link
Contributor

@robertsipka, precommit failed. Please fix it.

@robertsipka
Move 'nums_with_ascending_length' to be a global, non variable length…
88d7f2f 
… array.

VLAs cannot be initialized by any form of initialization syntax with C99 standard.
Also did some refactor around 'ecma_string_get_length' and 'ecma_string_get_size'.

JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
@robertsipka robertsipka force-pushed the remove_variable_length_arrays branch from fbcda55 to 88d7f2f Compare February 10, 2016 10:04
@robertsipka
Copy link
Contributor Author

@LaszloLango : There is no error with gcc 4.9.2. I checked with gcc 4.8.4, and the fail was occurred. I've fixed it, thanks!

@LaszloLango LaszloLango merged commit 88d7f2f into jerryscript-project:master Feb 10, 2016
@zherczeg zherczeg mentioned this pull request Sep 5, 2016
Closed
@robertsipka robertsipka deleted the remove_variable_length_arrays branch November 24, 2016 08:49
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
Projects
None yet
Milestone
Engine optimization & enhancement
Development

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