Skip to content

Conversation

@vmoroz
Copy link
Member

@vmoroz vmoroz commented Jun 2, 2023

The PR has the following set of changes:

  • Use new Hermes DLL that has only C-based API.
  • Use new V8 DLL that has new API matching the Hermes DLL API.
  • Use new JSI for Node-API implementation from node-api-jsi repo.
  • Remove use of non-ABI safe V8 JSI Runtime.
Microsoft Reviewers: Open in CodeFlow

@vmoroz vmoroz requested review from a team as code owners June 2, 2023 17:59
@vmoroz vmoroz marked this pull request as draft June 2, 2023 17:59
@vmoroz vmoroz changed the title (DRAFT) ABI-safe Hermes API and new JSI for Node-API ABI-safe Hermes API and new JSI for Node-API Jun 12, 2023
@vmoroz vmoroz marked this pull request as ready for review June 12, 2023 15:37
@vmoroz vmoroz requested a review from a team as a code owner June 12, 2023 15:37
@vmoroz vmoroz requested a review from a team June 12, 2023 15:40

class V8FuncResolver : public IFuncResolver {
public:
// TODO: (vmoroz) Use Office safe DLL loading
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we either do this, or file an issue to follow up on it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I will do it before merging.


m_jsiRuntime =
makeNodeApiJsiRuntime(env, &api, [runtime]() { CRASH_ON_ERROR(V8Api::current()->jsr_delete_runtime(runtime)); });
m_ownThreadId = std::this_thread::get_id();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just used for the validation in getRuntime right?
I dont think this is a valid check for fabric, since fabric will sometimes run the runtime the UI thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Let me see if I can remove it.

@acoates-ms
Copy link
Contributor

Looks good other than the thread id check.

@acoates-ms acoates-ms merged commit 00ef6df into microsoft:main Jun 13, 2023
vmoroz added a commit to vmoroz/microsoft-react-native-windows that referenced this pull request Jun 13, 2023
vmoroz added a commit to vmoroz/microsoft-react-native-windows that referenced this pull request Jun 14, 2023
acoates-ms pushed a commit that referenced this pull request Jun 14, 2023
* Fix Hermes sampling profiler (#11033)

* ABI-safe Hermes API and new JSI for Node-API (#11696)

* Update change files

* Update change file

* Fix compilation issues

* Fix Desktop compilation

* A few minor updates to .filters and comments
vmoroz added a commit to vmoroz/microsoft-react-native-windows that referenced this pull request Jun 14, 2023
vmoroz added a commit to vmoroz/microsoft-react-native-windows that referenced this pull request Jun 15, 2023
vmoroz added a commit to vmoroz/microsoft-react-native-windows that referenced this pull request Jun 20, 2023
acoates-ms pushed a commit that referenced this pull request Jun 20, 2023
* Revert "[0.71] ABI-safe Hermes API and new JSI for Node-API (#11696) (#11757)"

This reverts commit 7a340bd.

* Change files
vmoroz added a commit to vmoroz/microsoft-react-native-windows that referenced this pull request Aug 16, 2023
vmoroz added a commit that referenced this pull request Aug 17, 2023
* Re-apply cherry pick of PR #11696 with fixes

* Change files
JunielKatarn added a commit to jurocha-ms/react-native-windows that referenced this pull request Sep 15, 2023
JunielKatarn added a commit that referenced this pull request Sep 16, 2023
* Revert "[0.71] Update V8 JSI version to 0.71.8 (#12043)"

This reverts commit dcb75bb.

* Revert "[0.71] ABI-safe Hermes API and new JSI for Node-API (#11696) (#12033)"

This reverts commit fbed06a.

* Change files
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