Skip to content
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

Optimize TypedArrayObject.prototype.set, indexOf, lastIndexOf #1251

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

ksh8281
Copy link
Contributor

@ksh8281 ksh8281 commented Sep 11, 2023

  • Fix binary precedence bug

@ksh8281
Copy link
Contributor Author

ksh8281 commented Sep 11, 2023

Fix #1250

Comment on lines 492 to 497
if (type == TypedArrayType::BigInt64 || type == TypedArrayType::BigUint64) {
if (!value.isBigInt()) {
return Value(-1);
}
} else {
if (!value.isNumber()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about merging else and if to one else if statement here?
And if these conditions rarely happen, adding UNLIKELY would be good too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

if (type == TypedArrayType::BigInt64 || type == TypedArrayType::BigUint64) {
uint64_t argv0ToUint64 = type == TypedArrayType::BigInt64 ? bitwise_cast<uint64_t>(value.asBigInt()->toInt64()) : value.asBigInt()->toUint64();
while (compFn(byteK, byteLength)) {
if (*reinterpret_cast<uint64_t*>(&buffer[byteK]) == argv0ToUint64) {
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 fine to directly access array elements regardless of Endian?
What if we use BigEndian?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO We don't need to care endianness.
because two values are comes from same machine

Copy link
Contributor

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

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

LGTM

@clover2123 clover2123 merged commit ccb11a6 into Samsung:master Sep 12, 2023
24 checks passed
@ksh8281 ksh8281 mentioned this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants