Skip to content

Use binary search for keyword checking #2584

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
zherczeg merged 1 commit into from
Nov 8, 2018
Merged

Use binary search for keyword checking #2584

zherczeg merged 1 commit into from
Nov 8, 2018

Conversation

@rerobika
Copy link
Member

@rerobika rerobika commented Oct 31, 2018

This patch improves keyword searching during identifier parsing.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

@rerobika
Copy link
Member Author

rerobika commented Oct 31, 2018

Performance results:

Benchmark Perf (sec)
3d-cube.js 0.796 -> 0.796 : +0.064%
3d-raytrace.js 1.031 -> 1.026 : +0.540%
access-binary-trees.js 0.565 -> 0.564 : +0.331%
access-fannkuch.js 2.066 -> 2.066 : +0.027%
access-nbody.js 1.081 -> 1.078 : +0.267%
bitops-3bit-bits-in-byte.js 0.492 -> 0.491 : +0.320%
bitops-bits-in-byte.js 0.641 -> 0.642 : -0.172%
bitops-bitwise-and.js 0.917 -> 0.919 : -0.158%
bitops-nsieve-bits.js 1.110 -> 1.109 : +0.140%
controlflow-recursive.js 0.373 -> 0.365 : +2.323%
crypto-aes.js 0.874 -> 0.873 : +0.124%
crypto-md5.js 0.592 -> 0.592 : +0.035%
crypto-sha1.js 0.586 -> 0.586 : +0.077%
date-format-tofte.js 0.746 -> 0.744 : +0.354%
date-format-xparb.js 0.529 -> 0.523 : +1.104%
math-cordic.js 1.159 -> 1.156 : +0.244%
math-partial-sums.js 0.726 -> 0.724 : +0.277%
math-spectral-norm.js 0.535 -> 0.533 : +0.373%
string-base64.js 1.400 -> 1.398 : +0.139%
string-fasta.js 1.341 -> 1.338 : +0.188%
Geometric mean: +0.331%

Binary sizes (bytes)
74d4742:132664
2406026:132664

@LaszloLango
Copy link
Contributor

LaszloLango commented Oct 31, 2018

This PR has some overlapping with #2574

@rerobika rerobika force-pushed the lexer_check_keyword_optimize branch from 2406026 to 5d06d39 Compare October 31, 2018 14:42
zherczeg
zherczeg requested changes Nov 5, 2018
View reviewed changes
jerry-core/parser/js/js-lexer.c Outdated
{
const keyword_string_t *keyword_p; /**< keyword string list */
const uint8_t length; /**< keyword list length */
} keyword_string_list_t;
Copy link
Member

@zherczeg zherczeg Nov 5, 2018

Choose a reason for hiding this comment

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

It is true, that 8 bit length is more than enough, but the first member is a pointer, so the whole structure will be pointer aligned (so we waste 3 byte with 32 bit pointers). What about creating two arrays:

static const keyword_string_t * const keyword_strings_list[]
static const uint8_t keyword_lengths_list[]

(Note: an s added after string)

Copy link
Member Author

@rerobika rerobika Nov 5, 2018

Choose a reason for hiding this comment

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

It seems reasonable. Good idea!

zherczeg
zherczeg reviewed Nov 5, 2018
View reviewed changes
jerry-core/parser/js/js-lexer.c Outdated
* Keywords with 3 characters.
*/
static const keyword_string_t keyword_length_3[6] =
static const keyword_string_t keyword_length_3[] =
Copy link
Member

@zherczeg zherczeg Nov 5, 2018

Choose a reason for hiding this comment

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

This is just an idea, but what about renaming these to keywords_with_length_3 or keywords_of_length_3, I don't know which one is correct. Since we touch this code anyway. Or keyword_group_with_length_3. I am open to other suggestions.

Copy link
Member Author

@rerobika rerobika Nov 5, 2018

Choose a reason for hiding this comment

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

+1 for keywords_with_length_3.

@rerobika rerobika force-pushed the lexer_check_keyword_optimize branch from 5d06d39 to ccf32f5 Compare November 5, 2018 08:42
robertsipka
robertsipka reviewed Nov 6, 2018
View reviewed changes
jerry-core/parser/js/js-lexer.c Outdated
#define LEXER_KEYWORD(name, type) { (const uint8_t *) (name), (type) }
#define LEXER_KEYWORD_END() { (const uint8_t *) NULL, LEXER_EOS }
#define LEXER_KEYWORD_LIST_LENGTH(name) (const uint8_t) (sizeof ((name)) / sizeof ((name)[0]))
#define LEXER_KEYWORD_END()
Copy link
Contributor

@robertsipka robertsipka Nov 6, 2018

Choose a reason for hiding this comment

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

The LEXER_KEYWORD_END() is still needed?

Copy link
Member Author

@rerobika rerobika Nov 7, 2018

Choose a reason for hiding this comment

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

Good catch! It's a leftover code.

@rerobika rerobika force-pushed the lexer_check_keyword_optimize branch from ccf32f5 to 8a07389 Compare November 7, 2018 10:17
@rerobika
Copy link
Member Author

rerobika commented Nov 7, 2018

@zherczeg @robertsipka The patch is updated according to your suggestions, also rebased to the latest master.

zherczeg
zherczeg reviewed Nov 7, 2018
View reviewed changes
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.

Good patch, nearly done.

jerry-core/parser/js/js-lexer.c Outdated
if (relation == 0)
{
if (keyword_p->type >= LEXER_FIRST_FUTURE_STRICT_RESERVED_WORD)
relation = memcmp (ident_start_p, keyword_p->keyword_p, length);
Copy link
Member

@zherczeg zherczeg Nov 7, 2018

Choose a reason for hiding this comment

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

I would cal this compare_result

Copy link
Member Author

@rerobika rerobika Nov 7, 2018

Choose a reason for hiding this comment

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

True, sounds better.

@rerobika
Use binary search for keyword checking
41bcb42 
This patch improves keyword searching during identifier parsing.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu
@rerobika rerobika force-pushed the lexer_check_keyword_optimize branch from 8a07389 to 41bcb42 Compare November 7, 2018 10:33
@robertsipka
Copy link
Contributor

robertsipka commented Nov 7, 2018

Nice patch, LGTM.

zherczeg
zherczeg approved these changes Nov 8, 2018
View reviewed changes
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

@zherczeg zherczeg merged commit 1fc369e into jerryscript-project:master Nov 8, 2018
@rerobika rerobika deleted the lexer_check_keyword_optimize branch February 28, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zherczeg zherczeg zherczeg approved these changes

@robertsipka robertsipka robertsipka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rerobika @LaszloLango @robertsipka @zherczeg