[API] Improve the performance of the external magic id search #1506
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LaszloLango
requested changes
Jan 2, 2017
| @@ -157,6 +157,22 @@ lit_magic_strings_ex_set (const lit_utf8_byte_t **ex_str_items, /**< character a | |||
| lit_utf8_size_t string_size = lit_zt_utf8_string_size (lit_get_magic_string_ex_utf8 (id)); | |||
| JERRY_ASSERT (JERRY_CONTEXT (lit_magic_string_ex_sizes)[id] == string_size); | |||
| JERRY_ASSERT (JERRY_CONTEXT (lit_magic_string_ex_sizes)[id] <= LIT_MAGIC_STRING_LENGTH_LIMIT); | |||
|
|
|||
| /* Check the order of the strings */ | |||
There was a problem hiding this comment.
I think we should add more description here. What is the right order? Maybe with a small example.
bzsolt
force-pushed
the
search-ext-strings
branch
from
abf81a7 to
b4859e1
Compare
zherczeg
requested changes
Jan 3, 2017
| const lit_utf8_byte_t *prev_ex_string_p = lit_get_magic_string_ex_utf8 (prev_id); | ||
| const lit_utf8_byte_t *curr_ex_string_p = lit_get_magic_string_ex_utf8 (id); | ||
| const int string_compare = memcmp (prev_ex_string_p, curr_ex_string_p, string_size); | ||
| JERRY_ASSERT (string_compare < 0); |
There was a problem hiding this comment.
JERRY_ASSERT (memcmp (prev_ex_string_p, curr_ex_string_p, string_size) < 0)
| } | ||
| else if (found_size_range) | ||
| { | ||
| break; |
There was a problem hiding this comment.
Are you sure this is correct?
a
b
c
d
e
ff
gg
Search e.
First, you find d, and found_size_range is set to 1. Then you find ff, and the loop aborts. If I understand correctly.
After this patch, we have to provide external strings ordered by size and lexicographically. We can do this with jerry_parse_and_save_literals() (jerryscript-project#1500). JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]
bzsolt
force-pushed
the
search-ext-strings
branch
from
b4859e1 to
f6c6937
Compare
robertsipka
reviewed
Jan 4, 2017
| @@ -150,8 +150,8 @@ handler_construct (const jerry_value_t func_obj_val, /**< function object */ | |||
| */ | |||
| #define JERRY_MAGIC_STRING_ITEMS \ | |||
| JERRY_MAGIC_STRING_DEF (GLOBAL, global) \ | |||
There was a problem hiding this comment.
The global is already in the list of ECMA magic strings, so it doesn't have to be an external magic string because the defined magic string id will be found at first. But I think it could be removed in a follow-up patch, otherwise LGTM (informally).
janjongboom
added a commit
to janjongboom/jerryscript
that referenced
this pull request
Jan 10, 2017
Magic strings are expected to be sorted by length, then alphabetically after jerryscript-project#1506 landed. This breaks the mbedos5 target against JerryScript master as it emits the pins in the order that it finds the pins. This patch sorts the pin names. JerryScript-DCO-1.0-Signed-off-by: Jan Jongboom [email protected]
zherczeg
pushed a commit
that referenced
this pull request
Jan 11, 2017
Magic strings are expected to be sorted by length, then alphabetically after #1506 landed. This breaks the mbedos5 target against JerryScript master as it emits the pins in the order that it finds the pins. This patch sorts the pin names. JerryScript-DCO-1.0-Signed-off-by: Jan Jongboom [email protected]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After this patch, we have to provide external strings ordered by size and lexicographically.
We can do this with jerry_parse_and_save_literals() (#1500).
JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]