Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Avoid mutating the global Promise #135

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Avoid mutating the global Promise #135

wants to merge 14 commits into from

Conversation

juanca
Copy link

@juanca juanca commented Aug 6, 2018

Fixes #134

Object.keys(Promise).filter(isNotMethod).forEach(del(Promise));
toFastProperties(Promise);
toFastProperties(Promise.prototype);
const PromiseCopy = Object.assign({}, Promise);
Copy link
Author

Choose a reason for hiding this comment

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

Hmm.. This is not supported until Node 4.

Is there preferred method to checking in a polyfill?

@juanca
Copy link
Author

juanca commented Aug 6, 2018

Oh, boy. It's really red.

These are fast. Do we need them to be faster?
@juanca
Copy link
Author

juanca commented Aug 6, 2018

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
Copy link
Collaborator

ljharb commented Aug 6, 2018

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

@juanca
Copy link
Author

juanca commented Aug 6, 2018

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

  • Switch out bluebird from a package with a strict interface (es6-promise?)
  • Create a class that only interfaces the desired Promise methods (stack traces might suck)
  • Not worry about strict interface

Would appreciate your thoughts on this.

@ljharb
Copy link
Collaborator

ljharb commented Aug 6, 2018

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 juanca changed the title Copy Promise class before mutating Avoid mutating Promise global Aug 6, 2018
@juanca juanca changed the title Avoid mutating Promise global Avoid mutating the global Promise Aug 6, 2018
@juanca
Copy link
Author

juanca commented Aug 7, 2018

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
Copy link
Collaborator

ljharb commented Aug 7, 2018

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

@juanca
Copy link
Author

juanca commented Aug 7, 2018

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
Copy link
Author

juanca commented Aug 7, 2018

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
Copy link
Author

juanca commented Aug 7, 2018

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/

juanca added 9 commits August 7, 2018 13:51
e.g. utils/strict-promise-test.js
…unning

`isStrictEqual` is not part of the API: replaced with `strictEqual`
Ensure promise is either rejected or resolved correctly.

This was entirely missed because I forgot to ensure the tests were running. I was breaking the test suite by removing code from StrictPromise -- but not ensuring the strict-promise-test was failing appropriately.
@ljharb
Copy link
Collaborator

ljharb commented Aug 8, 2018

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
Copy link
Author

juanca commented Aug 8, 2018

Yeah, I had the same thought cross my mind.

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

juanca added a commit to juanca/strict-promise that referenced this pull request Aug 8, 2018
This was a recent implementation by me. Figured it should not live in Hypernova if it is actually useful -- the speed of Bluebird but a small (read: strict) interface.

airbnb/hypernova#135
@juanca
Copy link
Author

juanca commented Aug 8, 2018

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
Copy link
Author

juanca commented Aug 8, 2018

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

@juanca
Copy link
Author

juanca commented Aug 8, 2018

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
Copy link

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
Copy link

espretto commented Jan 9, 2023

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.

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.

TypeError: Promise.promisifyAll is not a function
4 participants