Skip to content

Conversation

sand1k
Copy link
Contributor

@sand1k sand1k commented Jun 3, 2015

Replaced contiguous array of literals with flexible literal storage.
The storage allows to effectively manage memory occupied by literals. Addition and removal of a literal ('eval' or 'new function' processing, garbage collection) could be done without reallocation of the whole literal array, hence it solves fragmentation problem.

CMakeLists.txt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

How this change relates to the patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appeared here as result of debugging. Actually, it doesn't relate to this patch. I'll revert it.

@egavrin egavrin added parser Related to the JavaScript parser ecma core Related to core ECMA functionality development Feature implementation labels Jun 3, 2015
@egavrin egavrin added this to the Core ECMA features milestone Jun 3, 2015
@sand1k
Copy link
Contributor Author

sand1k commented Jun 3, 2015

@egavrin, fixed code according to your notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comments on arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add all the missing comments throughout the patch and also description of the literal storage internals in wiki.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to fix comments with follow up patches - we can fix it now.

@egavrin egavrin assigned egavrin and unassigned sand1k Jun 9, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how to place comment on unmodified code part, but there are some variable definitions above, which, probably, could be removed:

 static ecma_char_t *strings_cache;
 static size_t strings_cache_size;
 static size_t strings_cache_used_size;

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

@egavrin
Copy link
Contributor

egavrin commented Jun 9, 2015

I've updated PR. @sand1k @ruben-ayrapetyan @galpeter please check.

@egavrin egavrin assigned galpeter and unassigned egavrin Jun 10, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that new whitespace here and a bit above is not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

@galpeter
Copy link
Contributor

Seems good to me (btw: we would also need to rebase this)

@egavrin
Copy link
Contributor

egavrin commented Jun 10, 2015

@galpeter thank you! We'll rebase and land it.

/cc @ruben-ayrapetyan

@sand1k sand1k force-pushed the Andrey-literal-storage branch from 377170f to 6fcb695 Compare June 10, 2015 18:20
sand1k added 3 commits June 10, 2015 21:40
JerryScript-DCO-1.0-Signed-off-by: Evgeny Gavrin [email protected]
JerryScript-DCO-1.0-Signed-off-by: Andrey Shitov [email protected]
JerryScript-DCO-1.0-Signed-off-by: Evgeny Gavrin [email protected]
JerryScript-DCO-1.0-Signed-off-by: Andrey Shitov [email protected]
JerryScript-DCO-1.0-Signed-off-by: Evgeny Gavrin [email protected]
JerryScript-DCO-1.0-Signed-off-by: Andrey Shitov [email protected]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical Raises security concerns development Feature implementation ecma core Related to core ECMA functionality normal parser Related to the JavaScript parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants