Skip to content
This repository was archived by the owner on Oct 15, 2020. It is now read-only.

chakrashim: Use JsCreateExternalObjectWithPrototype#472

Merged
kfarnung merged 1 commit intonodejs:masterfrom
kfarnung:pr429
Feb 22, 2018
Merged

chakrashim: Use JsCreateExternalObjectWithPrototype#472
kfarnung merged 1 commit intonodejs:masterfrom
kfarnung:pr429

Conversation

@kfarnung
Copy link
Copy Markdown
Contributor

Previously, Chakrashim was calling both JsCreateExternalObject and JsSetPrototype.
JsSetPrototype is expensive to lazy call. Use combined JsCreateExternalObjectWithPrototype instead

This depends on the changes in chakra-core/ChakraCore#4719 landing first to prevent TTD breaks.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

chakrashim

@kfarnung kfarnung self-assigned this Feb 21, 2018
@kfarnung
Copy link
Copy Markdown
Contributor Author

I adopted #429, assuming @obastemur is ok with this, we can close his PR.

return Local<Object>();
}
&prototype) != JsNoError) {
prototype = nullptr;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on this, I largely think it's unnecessary, but I landed on the safe side.

@kfarnung
Copy link
Copy Markdown
Contributor Author

kfarnung commented Feb 21, 2018

Copy link
Copy Markdown
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

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

Looks right to me

@obastemur
Copy link
Copy Markdown
Contributor

@kfarnung Thanks

Previously, Chakrashim was calling both JsCreateExternalObject and
JsSetPrototype. JsSetPrototype is expensive to lazy call. Use combined
JsCreateExternalObjectWithPrototype instead.

PR-URL: nodejs#472
Refs: nodejs#429
Reviewed-By: Taylor Woll <[email protected]>
Reviewed-By: Oguz Bastemur <[email protected]>
@kfarnung kfarnung merged commit f295207 into nodejs:master Feb 22, 2018
@kfarnung kfarnung deleted the pr429 branch February 22, 2018 01:10
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Feb 23, 2018
Previously, Chakrashim was calling both JsCreateExternalObject and
JsSetPrototype. JsSetPrototype is expensive to lazy call. Use combined
JsCreateExternalObjectWithPrototype instead.

PR-URL: nodejs#472
Refs: nodejs#429
Reviewed-By: Taylor Woll <[email protected]>
Reviewed-By: Oguz Bastemur <[email protected]>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Mar 6, 2018
Previously, Chakrashim was calling both JsCreateExternalObject and
JsSetPrototype. JsSetPrototype is expensive to lazy call. Use combined
JsCreateExternalObjectWithPrototype instead.

PR-URL: nodejs#472
Refs: nodejs#429
Reviewed-By: Taylor Woll <[email protected]>
Reviewed-By: Oguz Bastemur <[email protected]>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Mar 7, 2018
Previously, Chakrashim was calling both JsCreateExternalObject and
JsSetPrototype. JsSetPrototype is expensive to lazy call. Use combined
JsCreateExternalObjectWithPrototype instead.

PR-URL: nodejs#472
Refs: nodejs#429
Reviewed-By: Taylor Woll <[email protected]>
Reviewed-By: Oguz Bastemur <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants