Skip to content

Unify internal property creation #4373

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
Jan 12, 2021
Merged

Unify internal property creation #4373

zherczeg merged 1 commit into from
Jan 12, 2021

Conversation

@zherczeg
Copy link
Member

@zherczeg zherczeg commented Dec 23, 2020

No description provided.

@zherczeg zherczeg force-pushed the internal_prop branch 5 times, most recently from fb920ee to c9b1655 Compare December 28, 2020 17:12
@zherczeg zherczeg mentioned this pull request Dec 29, 2020
Open
lygstate
lygstate reviewed Dec 31, 2020
View reviewed changes
jerry-core/ecma/base/ecma-globals.h
/**
* Checks whether a property is raw data or accessor property
*/
#define ECMA_PROPERTY_IS_RAW(property) \
Copy link
Contributor

@lygstate lygstate Dec 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I like something like (property & 0b00011000), C doesn't support for 0b, we can use 0x instead.
Without the need of compare operator
Any definition of property bits?
property is 8 bits long, what's the meaning of each bits?

Copy link
Member Author

@zherczeg zherczeg Dec 31, 2020

Choose a reason for hiding this comment

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

Good bit magic is essential for high performance. This condition checks that the highest two bits are not set without using a temporary register. The alternative ´(x & 0xc0) != 0xc0´ requires two operations and a temporary register.
Property bits are described in ecma_property_flags_t.

@zherczeg zherczeg force-pushed the internal_prop branch 2 times, most recently from bd99a33 to b071da6 Compare January 7, 2021 10:11
rerobika
rerobika approved these changes Jan 7, 2021
View reviewed changes
Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

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

LGTM

@LaszloLango
Copy link
Contributor

LaszloLango commented Jan 11, 2021

Property creation and usage are hot paths in the engine. Does this patch have any effect on the performance?

LaszloLango
LaszloLango requested changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

Please update ./docs/04.INTERNALS.md as well.

@zherczeg
Unify internal property creation
136fa18 
Furthermore free up a bit in the property descriptor.

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
@zherczeg
Copy link
Member Author

zherczeg commented Jan 12, 2021

Benchmark Heap (bytes) Stack (bytes) Perf (sec)
3d-cube.js 10992 -> 10992 : 0.000% 14232 -> 14232 : 0.000% 1.132 -> 1.122 : +0.850%
3d-morph.js 289232 -> 289232 : 0.000% 1704 -> 1704 : 0.000% 1.318 -> 1.324 : -0.449%
3d-raytrace.js 117656 -> 117656 : 0.000% 2596 -> 2596 : 0.000% 1.326 -> 1.320 : +0.525%
access-binary-trees.js 27120 -> 27120 : 0.000% 3512 -> 3512 : 0.000% 0.778 -> 0.790 : -1.574%
access-fannkuch.js 1792 -> 1792 : 0.000% 1776 -> 1776 : 0.000% 2.546 -> 2.555 : -0.354%
access-nbody.js 4600 -> 4600 : 0.000% 1864 -> 1864 : 0.000% 1.519 -> 1.521 : -0.119%
bitops-3bit-bits-in-byte.js 1024 -> 1024 : 0.000% 1720 -> 1720 : 0.000% 0.703 -> 0.701 : +0.291%
bitops-bits-in-byte.js 984 -> 984 : 0.000% 1720 -> 1720 : 0.000% 1.015 -> 1.013 : +0.247%
bitops-bitwise-and.js 752 -> 752 : 0.000% 1472 -> 1472 : 0.000% 1.251 -> 1.238 : +1.060%
bitops-nsieve-bits.js 61184 -> 61184 : 0.000% 1720 -> 1720 : 0.000% 1.477 -> 1.476 : +0.094%
controlflow-recursive.js 1288 -> 1288 : 0.000% 63624 -> 63624 : 0.000% 0.520 -> 0.523 : -0.458%
crypto-aes.js 28040 -> 28040 : 0.000% 2312 -> 2312 : 0.000% 0.884 -> 0.887 : -0.367%
crypto-md5.js 70968 -> 70968 : 0.000% 1788 -> 1788 : 0.000% 0.742 -> 0.742 : +0.104%
crypto-sha1.js 46328 -> 46328 : 0.000% 1848 -> 1848 : 0.000% 0.764 -> 0.758 : +0.707%
crypto.js 51704 -> 51704 : 0.000% 3228 -> 3228 : 0.000% 8.684 -> 8.657 : +0.307%
date-format-tofte.js 8952 -> 8952 : 0.000% 2120 -> 2120 : 0.000% 1.012 -> 1.004 : +0.760%
date-format-xparb.js 15144 -> 15144 : 0.000% 2704 -> 2704 : 0.000% 0.820 -> 0.813 : +0.854%
deltablue.js 455544 -> 455544 : 0.000% 3636 -> 3636 : 0.000% 5.487 -> 5.465 : +0.403%
math-cordic.js 2048 -> 2048 : 0.000% 1720 -> 1720 : 0.000% 1.711 -> 1.709 : +0.098%
math-partial-sums.js 1512 -> 1512 : 0.000% 1696 -> 1696 : 0.000% 1.154 -> 1.156 : -0.144%
math-spectral-norm.js 3264 -> 3264 : 0.000% 1720 -> 1720 : 0.000% 0.739 -> 0.739 : +0.073%
raytrace.js 22424 -> 22424 : 0.000% 5104 -> 5104 : 0.000% 2.774 -> 2.791 : -0.597%
richards.js 8568 -> 8568 : 0.000% 2196 -> 2196 : 0.000% 0.314 -> 0.314 : +0.093%
string-base64.js 90104 -> 90104 : 0.000% 1768 -> 1768 : 0.000% 1.434 -> 1.420 : +1.038%
string-fasta.js 4400 -> 4400 : 0.000% 1704 -> 1704 : 0.000% 3.420 -> 3.425 : -0.133%
Geometric mean: 0.000% 0.000% +0.134%
Binary (bytes) master(99ec5b9) patch(136fa18) Diff
size 226976 226976 0 bytes
.rodata 20003 20003 0 bytes
.dynstr 448 448 0 bytes
.rel.plt 456 456 0 bytes
.interp 25 25 0 bytes
.dynsym 1008 1008 0 bytes
.gnu.hash 464 464 0 bytes
.text 198352 198312 -40 bytes
.comment 85 85 0 bytes
.shstrtab 226 226 0 bytes
.data 8 8 0 bytes
.ARM.exidx 8 8 0 bytes
.rel.dyn 16 16 0 bytes
.init 12 12 0 bytes
.got 244 244 0 bytes
.plt 716 716 0 bytes
.note.ABI-tag 32 32 0 bytes
.gnu.version_r 64 64 0 bytes
.bss 1575088 1575088 0 bytes
.fini 8 8 0 bytes
.hash 408 408 0 bytes
.gnu.version 126 126 0 bytes
.fini_array 4 4 0 bytes
.init_array 4 4 0 bytes
.dynamic 248 248 0 bytes
.eh_frame 4 4 0 bytes
.ARM.attributes 53 53 0 bytes

No major effect.

LaszloLango
LaszloLango approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@LaszloLango LaszloLango 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 6f0391d into jerryscript-project:master Jan 12, 2021
@zherczeg zherczeg deleted the internal_prop branch January 12, 2021 12:12
ossy-szeged
ossy-szeged reviewed Jan 12, 2021
View reviewed changes
jerry-core/ecma/operations/ecma-objects.c
Comment on lines +2418 to +2421
/* Internal properties are special properties so they must not be present. */
JERRY_ASSERT (ECMA_PROPERTY_GET_NAME_TYPE (*property_p) != ECMA_DIRECT_STRING_MAGIC
|| prop_pair_p->names_cp[i] < LIT_NON_INTERNAL_MAGIC_STRING__COUNT
|| prop_pair_p->names_cp[i] >= LIT_MAGIC_STRING__COUNT);
Copy link
Contributor

@ossy-szeged ossy-szeged Jan 12, 2021

Choose a reason for hiding this comment

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

This assert hits on debug test262-esnext on 116 tests. Please a take a look what happened here.

@ossy-szeged ossy-szeged mentioned this pull request Jan 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ossy-szeged ossy-szeged ossy-szeged left review comments

@LaszloLango LaszloLango LaszloLango approved these changes

@rerobika rerobika rerobika approved these changes

+1 more reviewer

@lygstate lygstate lygstate left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zherczeg @LaszloLango @lygstate @ossy-szeged @rerobika