Skip to content

Optimize LCache operation. #1116

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
Jun 2, 2016
Merged

Optimize LCache operation. #1116

merged 1 commit into from
Jun 2, 2016

Conversation

zherczeg
Copy link
Member

@zherczeg zherczeg commented Jun 1, 2016

The cache stores only real properties now, because storing NULLs has
little benefit according to tests. Since only real properties are
stored now, there is no need to create real references to objects
and property names, which reduces the keeping of dead objects after
garbage collection.

@zherczeg
Copy link
Member Author

zherczeg commented Jun 1, 2016 •
edited
Loading

Benchmark RSS (bytes) Perf (sec)
3d-cube.js 73728 -> 73728 : 0.000% 1.769 -> 1.486 : +16.018%
3d-raytrace.js 225280 -> 217088 : +3.636% 1.987 -> 1.522 : +23.385%
access-binary-trees.js 77824 -> 77824 : 0.000% 0.860 -> 0.765 : +11.047%
access-fannkuch.js 32768 -> 32768 : 0.000% 3.785 -> 3.182 : +15.919%
access-nbody.js 32768 -> 32768 : 0.000% 2.109 -> 1.558 : +26.136%
bitops-3bit-bits-in-byte.js 28672 -> 24576 : +14.286% 0.828 -> 0.793 : +4.232%
bitops-bits-in-byte.js 24576 -> 24576 : 0.000% 1.096 -> 1.041 : +4.968%
bitops-bitwise-and.js 28672 -> 24576 : +14.286% 1.879 -> 1.935 : -2.983%
bitops-nsieve-bits.js 167936 -> 159744 : +4.878% 2.736 -> 2.501 : +8.583%
controlflow-recursive.js 118784 -> 114688 : +3.448% 0.553 -> 0.546 : +1.194%
crypto-aes.js 110592 -> 98304 : +11.111% 1.928 -> 1.470 : +23.762%
crypto-md5.js 180224 -> 180224 : 0.000% 0.985 -> 0.919 : +6.744%
crypto-sha1.js 118784 -> 118784 : 0.000% 0.916 -> 0.865 : +5.623%
date-format-tofte.js 61440 -> 53248 : +13.333% 1.437 -> 1.161 : +19.200%
date-format-xparb.js 57344 -> 53248 : +7.143% 0.653 -> 0.597 : +8.440%
math-cordic.js 28672 -> 28672 : 0.000% 2.076 -> 1.964 : +5.435%
math-partial-sums.js 28672 -> 28672 : 0.000% 1.122 -> 1.065 : +5.082%
math-spectral-norm.js 32768 -> 32768 : 0.000% 0.957 -> 0.884 : +7.644%
string-base64.js 151552 -> 147456 : +2.703% 4.923 -> 4.683 : +4.874%
string-fasta.js 40960 -> 40960 : 0.000% 2.357 -> 2.231 : +5.374%
Geometric mean: +3.887% +10.390%

Binary sizes (bytes)
old:167752
new:167392

@LaszloLango
Copy link
Contributor

Impressive results :)

@LaszloLango LaszloLango added enhancement An improvement performance Affects performance memory consumption Affects memory consumption binary size Affects binary size labels Jun 1, 2016
LaszloLango
LaszloLango reviewed Jun 1, 2016
View reviewed changes
jerry-core/ecma/base/ecma-lcache.c
if (!is_cached)
for (uint32_t entry_index = 0; entry_index < ECMA_LCACHE_HASH_ROW_LENGTH; entry_index++)
{
if (entry_p->object_cp == object_cp && entry_p->prop_p == prop_p)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to attach the same property to different objects? What I'm trying to say here is can the following code work here?

if (entry_p->prop_p == prop_p)
{
  JERRY_ASSERT (entry_p->object_cp == object_cp);

Copy link
Member Author

Choose a reason for hiding this comment

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

No it is not possible. But we need to check that entry_p->object_cp cannot be not NULL. Hm, but this could be a good idea. Since we only cache valid properties now, we could move the NULL to the property pointer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about it, and moving the NULL to prop_p is inefficient because the lookup() function depends on the object_cp. Here a (entry_p->prop_p == prop_p) check is not enough, since a property can be added, removed, and added to another entry position. This could work though:

if (entry_p->object_cp != ECMA_NULL_POINTER && entry_p->prop_p == prop_p)

But I think the difference is minor compared to the other one. I will try it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zherczeg, I see. Thanks for checking.

@zherczeg zherczeg force-pushed the lookup_cache_opt branch 3 times, most recently from 82a994d to 2e6343f Compare June 2, 2016 08:21
LaszloLango
LaszloLango reviewed Jun 2, 2016
View reviewed changes
jerry-core/ecma/base/ecma-lcache.c
}
}
} /* ecma_lcache_invalidate_row_for_object_property_pair */
return (size_t) ((ecma_string_hash (prop_name_p) ^ object_cp) & (ECMA_LCACHE_HASH_ROWS_COUNT - 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please help me to understand this line. Maybe some comment would be good. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is randomizing the hash with the object. Before this patch, objects with the same field names evicted each other:

a[0] = 1; b[0] = 1; c[0] = 1; // c[0] evicted a[0] before. Not anymore

This happened frequently in access-fannkuch test. Now because the hash is xor-ed with the object, properties do not evict each other anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's tricky :) Great work!

@dbatyai
Copy link
Member

dbatyai commented Jun 2, 2016

Could we use the cache in vm_op_set_value as well, for example when reassign a value to a property, or is something preventing this?

@LaszloLango
Copy link
Contributor

LGTM

@zherczeg zherczeg force-pushed the lookup_cache_opt branch from 2e6343f to bbd71ed Compare June 2, 2016 09:24
@zherczeg
Copy link
Member Author

zherczeg commented Jun 2, 2016

Actually ecma_op_object_put in vm_op_set_value has been recently optimized and already using it. But we could probably do something here as well in a follow-up patch.

dbatyai
dbatyai reviewed Jun 2, 2016
View reviewed changes
jerry-core/ecma/base/ecma-lcache.h
extern void ecma_lcache_insert (ecma_object_t *, ecma_string_t *, ecma_property_t *);
extern bool ecma_lcache_lookup (ecma_object_t *, const ecma_string_t *, ecma_property_t **);
extern ecma_property_t * ecma_lcache_lookup (ecma_object_t *, const ecma_string_t *);
Copy link
Member

Choose a reason for hiding this comment

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

Extra space after asterix

@zherczeg
Optimize LCache operation.
5c852ab 
The cache stores only real properties now, because storing NULLs has
little benefit according to tests. Since only real properties are
stored now, there is no need to create real references to objects
and property names, which reduces the keeping of dead objects after
garbage collection.

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com
@zherczeg zherczeg force-pushed the lookup_cache_opt branch from bbd71ed to 5c852ab Compare June 2, 2016 10:56
@dbatyai
Copy link
Member

dbatyai commented Jun 2, 2016

LGTM

@zherczeg zherczeg merged commit 5c852ab into jerryscript-project:master Jun 2, 2016
@zherczeg zherczeg deleted the lookup_cache_opt branch June 2, 2016 11:15
@zherczeg zherczeg mentioned this pull request Jul 20, 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
binary size Affects binary size enhancement An improvement memory consumption Affects memory consumption performance Affects performance
Projects
None yet
Milestone
Engine optimization & enhancement
Development

Successfully merging this pull request may close these issues.
3 participants
@zherczeg @LaszloLango @dbatyai