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

The package "@tensorflow/tfjs-backend-wasm" is not minifier-safe #4253

Closed
evanw opened this issue Nov 17, 2020 · 8 comments · Fixed by #4392
Closed

The package "@tensorflow/tfjs-backend-wasm" is not minifier-safe #4253

evanw opened this issue Nov 17, 2020 · 8 comments · Fixed by #4392
Assignees
Labels

Comments

@evanw
Copy link

evanw commented Nov 17, 2020

I'm working on a bug filed by a user of this library against esbuild, a bundler I work on. The @tensorflow/tfjs-backend-wasm package breaks when bundled and minified. This problem is not specific to esbuild; it happens with a normal Webpack production build too with all default options. However, this is a problem that needs to be solved in the library itself instead of by the bundler, so I figured I should file an issue here about this.

System information

  • Have I written custom code (as opposed to using a stock example script provided in TensorFlow.js): no
  • OS Platform and Distribution (e.g., Linux Ubuntu 16.04): macOS 10.15.7
  • Mobile device (e.g. iPhone 8, Pixel 2, Samsung Galaxy) if the issue happens on mobile device: N/A
  • TensorFlow.js installed from (npm or script link): npm
  • TensorFlow.js version (use command below): ?
  • Browser version: Chrome 80
  • Tensorflow.js Converter Version: N/A

Describe the current behavior

The @tensorflow/tfjs-backend-wasm package sometimes breaks when minified. This is the case with Webpack and other bundlers.

The specific reason for the breakage is described here: evanw/esbuild#538 (comment). At a high level, this library converts a function to a string and then converts that string back to a function. This essentially "rips" it out of its local scope and injects it into another scope and then re-binds all identifiers. That function accesses a local variable called _scriptDir and expects that variable to keep the same name in the function source code.

Some ways of fixing this:

  • This could be fixed by placing the code that should not be minified inside a string instead of using a function expression to contain the code. This will ensure that the minifier will not transform it in any way.

  • This could be fixed by not using the name of the variable to implement this binding. It would be best to pass the value of this variable as an additional parameter to the function so the function doesn't reach for anything outside of its scope.

  • There is an alternative hack using direct eval if doing this is not possible. This hack is also described in esbuild minify mismanages some border cases evanw/esbuild#538 (comment). Using direct eval is an anti-pattern so I'd only do this if other options don't work.

Describe the expected behavior

The @tensorflow/tfjs-backend-wasm package should not behave differently when minified.

Standalone code to reproduce the issue

There is a full reproduction case here: evanw/esbuild#538 (comment). Note that to hit the issue, you need to have both WebAssembly SIMD support and WebAssembly threads support enabled in chrome://flags/.

@evanw evanw added the type:bug Something isn't working label Nov 17, 2020
@rthadur
Copy link
Contributor

rthadur commented Nov 17, 2020

cc @tafsiri @annxingyuan

@tafsiri tafsiri added the RELEASE BLOCKER These issues must be resolved before the next release. label Nov 18, 2020
@simon-lanf
Copy link

I use @tensorflow/tfjs-backend-wasm with webpack and I have no problems. Webpack 4. I can give you my webpack config if you need.

@tafsiri
Copy link
Contributor

tafsiri commented Nov 18, 2020

@annxingyuan i can repro this issue (or at least one that seems related) with the sample webpack project in the repo. For the code mentioned in the issue, it this part of emscripten's output?

@annxingyuan
Copy link
Contributor

@tafsiri Yes, the code is part of emscripten's output. However we prepend code to it when converting it to a blob.

@annxingyuan
Copy link
Contributor

@tafsiri Do you think this needs the release blocker tag since it is not specific to master?

@tafsiri
Copy link
Contributor

tafsiri commented Nov 19, 2020

Hmm I was thinking so because the example in the repo doesn't work in current form? What do you think?

I think with a closer look we can decide whether we want to this to block the next release (for example if we have to file an upstream bug with emscripten). But thought we should at least investigate before doing another release.

@annxingyuan
Copy link
Contributor

Work-in-progress PR: emscripten-core/emscripten#12832

@tafsiri tafsiri removed the RELEASE BLOCKER These issues must be resolved before the next release. label Nov 20, 2020
@google-ml-butler
Copy link

Are you satisfied with the resolution of your issue?
Yes
No

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 a pull request may close this issue.

5 participants