airbnb / hypernova

A service for server-side rendering your JavaScript views
MIT License
5.82k stars 207 forks source link

Avoid mutating the global Promise #135

Open juanca opened 6 years ago

juanca commented 6 years ago

Fixes https://github.com/airbnb/hypernova/issues/134

juanca commented 6 years ago

Oh, boy. It's really red.

juanca commented 6 years ago

I decided to remove any modifications to the bluebird promise. I wasn't able to justify modifying the promise class -- it was probably done for speed? strict interface? I intuit that there are better ways to achieve either of those.

ljharb commented 6 years ago

It's done to ensure a strict interface; portable code never uses bluebird methods off of the global Promise.

juanca commented 6 years ago

I'm thinking we can do a few things then:

Would appreciate your thoughts on this.

ljharb commented 6 years ago

Bluebird is faster in v8 (node/chrome), but is slower in other browsers - so it's great to use in hypernova on the server, but it shouldn't ever be shipped to a browser environment. That's part of the reason that you'd want to avoid relying on it in your browser code.

If there were a way to use/wrap a distinct Bluebird instance and retain the strict interface, that'd be great - but imo the best solution here is to migrate away from relying on Bluebird in your own code.

juanca commented 6 years ago

Bluebird is faster in v8 (node/chrome), but is slower in other browsers - so it's great to use in hypernova on the server, but it shouldn't ever be shipped to a browser environment. That's part of the reason that you'd want to avoid relying on it in your browser code.

Good to know!

If there were a way to use/wrap a distinct Bluebird instance and retain the strict interface, that'd be great - but imo the best solution here is to migrate away from relying on Bluebird in your own code.

I'll try implement something with a strict interface. Unfortunately, it seems that various Webpack plugins already use the extensive Bluebird interface.

ljharb commented 6 years ago

Why would webpack plugins run in the same context as hypernova?

juanca commented 6 years ago

In my development environment, I am spinning up a Webpack process that wraps Hypernova as a means of having the most up-to-date manifest.json

Concretely:

webpack(config, (err, stats) => {
  if (err) {
    console.error(err);
    return;
  }

  const entries = Object.keys(stats.compilation.namedChunks);
  const entriesJSFilesMap = entries.reduce((map, entry) => ({
    ...map,
    [entry]: generateJSFilePath(stats, entry),
  }), {});

  console.log(stats.toString({
    modules: false,
  }));

  console.log(entriesJSFilesMap);

  hypernova({
    devMode: true,

    getComponent(name) {
      return require(entriesJSFilesMap[name]).default; // eslint-disable-line
    },
  });
});

While in the production environment, the manifest.json is already available (and static) on disk.

Is there a better way to go about setting up a Webpack + Hypernova environment?

juanca commented 6 years ago

I just realized that the set up is broken because I am spinning up a new Hypernova process on file changes. But a few tweaks and we're still relatively good-to-go.

Updated setup:

const entriesJSFilesMap = {};

webpack(config, (err, stats) => {
  if (err) {
    console.error(err);
    return;
  }

  const entries = Object.keys(stats.compilation.namedChunks);
  entries.reduce((map, entry) => ({
    ...map,
    [entry]: generateJSFilePath(stats, entry),
  }), entriesJSFilesMap);

  console.log(stats.toString({
    modules: false,
  }));
});

hypernova({
  devMode: true,

  getComponent(name) {
    return require(entriesJSFilesMap[name]).default; // eslint-disable-line
  },
});
juanca commented 6 years ago

I went ahead and implemented StrictPromise.

I need to backfill the tests for the static methods.

I also dropped cast since it is deprecated.

EDIT: coverage is missing an appropriate glob for running the new test file under utils/

ljharb commented 6 years ago

This is certainly a cleaner approach; but I'm worried this will preclude the very performance optimizations that have us using Bluebird in the first place.

juanca commented 6 years ago

Yeah, I had the same thought cross my mind.

Are there some benchmarks we can run to get some data behind these ideas?

juanca commented 6 years ago

I ran the doxbee benchmark from Bluebird.

Here are the results (paraphrased for the important bits):

results for 10000 parallel executions, 1 ms per I/O op

file                                       time(ms)  memory(MB)
callbacks-baseline.js                           180       27.42
promises-bluebird.js                            287       47.13
promises-ecmascript6-native.js                  553      100.14
promises-strict-promise.js                      911      123.78
observables-baconjs-bacon.js.js               31873      745.43

Platform info:
Darwin 17.5.0 x64
Node.JS 8.11.1
V8 6.2.414.50
Intel(R) Core(TM) i5-6287U CPU @ 3.10GHz × 4
juanca commented 6 years ago

I'll give it a few more stabs and see if I can optimize it.

juanca commented 6 years ago

The fastest I could get it down to was ~800ms @ 100mb.

Though, something interesting to note:

I switched out the StrictPromise class for Bluebird's Promise in the same ES6 benchmark setup. Here are the results (same computer):

promises-bluebird.js                            409       73.23
promises-ecmascript6-native.js                  518       95.57

Bluebird is only really ~20% faster than Native ES6 Promises. The original benchmarks are skewed in favor of Bluebird because it is using a non-ES6 interface. In the case of Hypernova, I think it stands to reason that there is only a 20% performance gain over native.

I might peruse Bluebird's repo for smart tips and tricks but it might not be possible to wrap Bluebird without incurring a significant performance loss (>50%) -- might fork and extract.

I'll try to fix my problems by experimenting with Webpack + Hypernova parallel processing for the time being.

Though, I am still interested in exploring the idea of not mutating Bluebird for the sake of code quality and instead accomplish the same feat via other methods (e.g. linting?). Let me know how you would like to proceed.

tylerhunt commented 5 years ago

FWIW, I ran into this same issue today trying to set up a Hypernova server based on my Rails webpacker configuration, although the error message was slightly different:

TypeError: BB.promisify is not a function
    at Object.<anonymous> (/node_modules/cacache/lib/util/fix-owner.js:5:19)

The comment here is what first led me down this path. Looks like bluebird is a dependency of cacache, which in turn is a dependency of webpack plugins like compression-webpack-plugin and uglifyjs-webpack-plugin.

I understand it isn't a common situation to be loading webpack alongside Hypernova, but I'd still love to see some kind of resolution on this.

espretto commented 1 year ago

Hello, the issue has been dormant for 3+ years now. Can anyone confirm the performance gains of Bluebird are actually worth the maintenance cost (#183 , #201)? Is there any interest in compatibility below node v10? I'd like to see hypernova simply use the native Promise.