Skip to content

Turn some global variables static const #892

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 17, 2016
Merged

Turn some global variables static const #892

merged 1 commit into from
Feb 17, 2016

Conversation

akosthekiss
Copy link
Member

Generally, it helps the optimizing passes of the compiler if global
varibles are static, and it is good for RAM usage to have data
marked read-only const. Found some globals, which could benefit
from these qualifiers.

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu

@LaszloLango LaszloLango added enhancement An improvement memory consumption Affects memory consumption labels Feb 17, 2016
@LaszloLango
Copy link
Contributor

@bzsolt, could you measure this patch with MEM_STATS builds on RPi2? @akiss77, I think we can find a few more from this. We should check it and extend this PR if we found more.

@zherczeg
Copy link
Member

LGTM

zherczeg
zherczeg reviewed Feb 17, 2016
View reviewed changes
jerry-core/parser/js/js-lexer.c
{
(const uint8_t *) "get", 3, LEXER_IDENT_LITERAL, PARSER_FALSE
};

static lexer_lit_location_t lexer_set_literal =
static const lexer_lit_location_t lexer_set_literal =
Copy link
Member

Choose a reason for hiding this comment

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

A qestion: the compiler should complain that lexer_same_identifiers arguments should be const. Did you run make precommit?

@akosthekiss
Turn some global variables static const
ad6107b 
Generally, it helps the optimizing passes of the compiler if global
varibles are `static`, and it is good for RAM usage to have data
marked read-only `const`. Found some globals, which could benefit
from these qualifiers.

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu
@akosthekiss
Copy link
Member Author

I've measured the read-only and read/write section sizes in the built binaries, both on current master and on this PR. Results below. (I've expected more data savings from the const modifiers...)

filename master/text master/data pr/text pr/data diff/text diff/data
debug.linux 854328 6996 855608 5780 1280 -1216
debug.linux-cp 664785 6708 666193 5492 1408 -1216
debug.linux-cp_minimal 455611 5968 457019 4688 1408 -1280
debug.mcu_stm32f4-cp 859044 3732 860300 2484 1256 -1248
debug.mcu_stm32f4-cp_minimal 507196 3520 508456 2272 1260 -1248
release.linux 288719 1405 288783 1373 64 -32
release.linux-cp 219266 1037 219330 989 64 -48
release.linux-cp_minimal 135007 268 135071 204 64 -64
release.mcu_stm32f3-cp 145744 280 145768 264 24 -16
release.mcu_stm32f3-cp_minimal 82784 84 82800 68 16 -16
release.mcu_stm32f4-cp 145744 280 145768 264 24 -16
release.mcu_stm32f4-cp_minimal 82784 84 82800 68 16 -16

@LaszloLango I went trough all the global variables that were listed by the doxygen documentation and found only these with incorrect (or suboptimal) qualifiers.

@zherczeg I've built both master and this PR with make clean && make and all went fine. (At least no errors have been reported. I'm not sure about warnings. But we already have quite a lot of them turned into errors, so I consider the builds OK.)

@LaszloLango
Copy link
Contributor

LGTM

@bzsolt
Copy link
Member

bzsolt commented Feb 17, 2016

RPi2 results by tools/run-mem-stats-test.sh
master (0d7ea70)

The results are the same, except in one case. The parenthesized value shows the result on the master branch.

               Test name    Peak Heap (parser)    Peak Heap (execution)  Maximum RSS
                 3d-cube              6152             21015              86016
     access-binary-trees              1255             45999              73728
         access-fannkuch              1501              5024              28672
            access-nbody              2890              7702              36864
bitops-3bit-bits-in-byte               886               828              24576
     bitops-bits-in-byte               798               802              24576
      bitops-bitwise-and               574               578              24576
   controlflow-recursive               852             11032             196608
              crypto-aes             12691             70808             114688
              crypto-md5              7573            141720             180224 (176128)
             crypto-sha1              4685             90921             126976
       date-format-tofte              5466             20329              53248
       date-format-xparb             10380             20930              61440
             math-cordic              1739              2135              28672
       math-partial-sums              1181              1653              24576
      math-spectral-norm              1269              6973              32768
            string-fasta              2393             13093              40960

@akosthekiss akosthekiss merged commit ad6107b into jerryscript-project:master Feb 17, 2016
@akosthekiss akosthekiss deleted the static-const-globals branch February 17, 2016 10:56
@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 memory consumption Affects memory consumption
Projects
None yet
Milestone
Engine optimization & enhancement
Development

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