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

Make tfjs-node create a new TFJSBackend for each environment #5108

Merged
merged 3 commits into from May 24, 2021

Conversation

bmcdorman
Copy link
Contributor

@bmcdorman bmcdorman commented May 21, 2021

This fixes issue #4947.

tfjs-node assumes a single global TFJSBackend for all napi_envs. This assumption holds for most applications, but not when embedding node in C++ with multiple environments or when using web workers. The user observes typical non-deterministic behavior with random crashes and weird error messages.

To resolve this, a map is created that holds a TFJSBackend for each napi_env.

Cleanup wasn't handled by the previous global implementation, so it's also not handled by this implementation. This may be undesirable. I also did this quickly to fix an issue I was having with little regard for style, DRYness, or other considerations, so hope it's a useful starting point for a project developer at least.


This change is Reviewable

@google-cla google-cla bot added the cla: yes label May 21, 2021
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Thank you very much for the contribution. Our team is not specialized in the node.js development, sorry that we missed the clean up logic for the backend instance. We would love to hear more about your thoughts on the backend clean up, how we should hook up with the node napi_env tear down.

Reviewable status: 0 of 1 approvals obtained (waiting on @bmcdorman)


tfjs-node/binding/tfjs_binding.cc, line 79 at r1 (raw file):

Quoted 7 lines of code…
  TFJSBackend *backend = nullptr;
  {
    std::lock_guard<std::mutex> lock(gBackendsMut);
    const auto it = gBackends.find(env);
    if (it == gBackends.cend()) NAPI_THROW_ERROR(env, "Backend not found for environment");
    backend = it->second;
  }

I will be good to extract this logic into a separate method to reduce code duplication.

@bmcdorman
Copy link
Contributor Author

bmcdorman commented May 22, 2021

It looks like there's actually a better way to go about this with Node's Environment Lifecycle API. That'll remove the lock and also make cleanup trivial to implement. I'll take another stab at it in the next couple days.

Thanks for the feedback!

…TFJSBackends when the environment is deallocated
@bmcdorman
Copy link
Contributor Author

Okay, cleanup has been added and we're now using the Node Environment Lifecycle API. Removing the lock also sped things up a little for me in my testing (with 3 environments hammering tfjs-node). I think it's good to go.

@bmcdorman bmcdorman requested a review from pyu10055 May 24, 2021 12:26
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Environment Lifecycle API
Looks great, thanks!

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @bmcdorman)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants