-
Notifications
You must be signed in to change notification settings - Fork 675
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.
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
literal storage: 4x performance improvement using a hashmap #5131
base: master
Are you sure you want to change the base?
Conversation
JerryScript-DCO-1.0-Signed-off-by: Ronan Jezequel [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A 4x times speedup is pretty nice! Nice job!
The patch looks good. I am not sure I understand the "unlicense" license of the original work.
hashmap_uint32_t initial_capacity; /**< initial hashmap capacity */ | ||
} hashmap_create_options_t; | ||
|
||
/// @brief Create a hashmap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this type of comment is used in jerryscript
|
||
/* | ||
The lit-hashmap library is based on the public domain software | ||
available from https://github.com/sheredom/hashmap.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license of https://github.com/sheredom/hashmap.h, the "Unlincense" is problematic: https://groups.google.com/g/unlicense/c/G6pzAFtgrx0/m/9TCxE-je2qMJ
Unlicense will not be reviewed by the OSI because it is a "crayon" licence (i.e. drafted by non legal professionals). Such licences have been problematic in the past.
Although Unlicense will not be reviewed, some (supposed) flaws have been
highlighted.
https://web.archive.org/web/20130421094711/http://projects.opensource.org/pipermail/license-review/2012-January/000052.html
In general the Apache or MIT licenses are recommended instead of various "public domain" licenses such as this Unlicense or CC0.
ok, let me rewrite the hashmap library, and at the same time use the standard comment syntax from jerryscript. |
JerryScript-DCO-1.0-Signed-off-by: Ronan Jezequel [email protected]
New commit added, with hashmap completely written from scratch - and also with better performance. |
Looks like a nice patch! We check it further soon |
It looks like tests/jerry/arithmetics.js fails. Do you know why? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please squash the commits and add DCO to the commit message
@ronanj I apologize for the delay, the CI issues for OSX have been resolved, please squash and rebase your changes, and add DCO to the commit message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in my previous comment, the CI issues for OSX have been resolved. Please squash and rebase your changes, and add DCO to the commit message.
Close and reopen to re-trigger the CI |
@ronanj could you please rebase the PR? It will make the CI green (probably). |
I have some idea about this, currently, the ecma_string_t is super complicated and have many issue about that, I'd like to take the idea Atom from quickjs to implement |
This PR is used to improve the literal storage performance:
Currently, literals are stored using a linked list: The drawback of the linked list is that when the number of literal entries increases, the time it takes to add a new entry increases proportionally to the number of entries: The reason is that before adding new entries, the storage engine first needs to iterate over all existing entries to find out if the "new" entry is a duplicate of an existing one or a new entity.
When running on a 200 Mhz CPU with a heap stored in PSRAM, if 1000 entries are already in the literal storage linked list, then it takes almost 1 millisecond to add a new entry (eg 1 millisecond to iterate over all existing 1000 entries). This makes loading snapshots very slow, especially when many background modules have already been loaded (where each background module adds its literals). Empirically, we measured that it takes 200 milliseconds for
jerry_exec_snapshot
to load and execute a snapshot containing about 200 literals when 1000 literals are already in the list before loading (measured on 200Mhz CPU with heap in PSRAM).The change introduces an additional hashmap to speed up the look-up of existing literals: This way, it is not necessary to iterate through the entire literal linked list (in O(n)) - and instead, an O(1) lookup operation can be used. Empirically, we have noticed that the application that took 200 milliseconds to load now takes 50 ms (measured from
jerry_exec_snapshot
), bringing a 4 times performance improvement.This feature can be enabled using the flag
JERRY_LIT_HASHMAP
. The test configuration has been updated to include this feature flag. Unit and functional tests have successfully passed. Test has also been done on the Open Harmony Jerry script version.JerryScript-DCO-1.0-Signed-off-by: Ronan Jezequel [email protected]