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

cleanup(wip): remove bluebird #5511

Closed
wants to merge 1 commit into from
Closed

cleanup(wip): remove bluebird #5511

wants to merge 1 commit into from

Conversation

talentlessguy
Copy link

What does it do?

Removes Bluebird and replaces it with native Promise API. Promises have been in Node since 0.12 so there's no need to use it anymore.

Needs a PR to https://github.com/hexojs/warehouse as well

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

Copy link

How to test

git clone -b remove-bluebird https://github.com/talentlessguy/hexo.git
cd hexo
npm install
npm test

@SukkaW
Copy link
Member

SukkaW commented Jun 30, 2024

#5215 (comment)

@talentlessguy
Copy link
Author

talentlessguy commented Jun 30, 2024

Ah crap, didn't see this. I'll switch to nativebird instead. Or close the PR?

@SukkaW
Copy link
Member

SukkaW commented Jun 30, 2024

Ah crap, didn't see this. I'll switch to nativebird instead. Or close the PR?

You can give nativebird a shot, and we can compare the performance. If there is no performance regression, we can continue!

@talentlessguy
Copy link
Author

talentlessguy commented Jun 30, 2024

@SukkaW oh awesome, I'll revert some of my changes then and use nativebird

@talentlessguy
Copy link
Author

Temporarily blocked by doodlewind/nativebird#13

And a few other methods which implementations of I'll add to nativebird later

@benjamingr
Copy link

Just refactor the bb specific stuff instead of wrapping it? You don't need Promise.asCallback when you have util.callbackify built in, Promise.promisify with util.promisify etc

@SukkaW
Copy link
Member

SukkaW commented Jul 1, 2024

Just refactor the bb specific stuff instead of wrapping it? You don't need Promise.asCallback when you have util.callbackify built in, Promise.promisify with util.promisify etc

The Hexo was born way before ES Promise was available/stable, so way too many Bluebird-specific API usages here and there in the Hexo codebase. A wrapper can ease the transition.

@benjamingr
Copy link

@SukkaW IMO just rip the bandaid, bluebird isn't getting major updates. We worked hard to make sure there are alternatives for every API we had in bluebird (I'm a maintainer) in Node.js (I'm.. also a maintainer).

Most stuff should just be refactored to native (e.g. Promise.method to async functions), other stuff to platform alternatives e.g. AbortSignal/AbortController for cancellation, util.promisify for converting callback functions, streams for map+concurrency etc.

The amount of stuff actually missing is very small (stuff like typed catch) and can be relatively easily worked around.


Also yes, I realize bluebird has no dependencies, we didn't want risk and to slow things down in the library - the library also has internal build tooling to reduce its size and improve its speed by not having a lot of runtime checks :]

@SukkaW
Copy link
Member

SukkaW commented Jul 1, 2024

IMO just rip the bandaid, bluebird isn't getting major updates. We worked hard to make sure there are alternatives for every API we had in bluebird (I'm a maintainer) in Node.js (I'm.. also a maintainer).

What I am really curious about is the bluebird performance. We've tried to replace bluebird 4 years ago: #3939, #4019. Our internal benchmark shows that there was about 175% performance regression at the moment.

To this day, bluebird still has comparable (where Bluebird is 2% faster) performance with the native promise in sequential executing while consuming 50% of the memory:

image

The benchmark suite can be found here: https://github.com/SukkaW/promise-performance-tests (originated from v8's promise benchmark suite).

Also, Hexo heavily relies on parallel job queueing, and as shown above, Bluebird's Promise.all is still 40% faster than the native Promise.all and consumes only 10% of the memory. That's why I always hesitated about replacing Bluebird.

And I know native Promise.all will always be slower since the native Promise.all is bound to the ECMAScript spec, while the Bluebird is only bound to the Promise A+ spec without strict runtime checks.

@benjamingr IMHO v8 could put more effort into improving Promise's performance. It has been 5 years and yet Bluebird still performs so well.

@benjamingr
Copy link

V8's benchmarks actually originate from Bluebird's which obviously show how good our promise performance is in various cases that may or may not matter to users :D Doxbee is actually pretty good and isolates promise overhead in a real world servers but if you look at the numbers from 10 years ago the numbers we have today (in both cases) are "so good" it shouldn't matter.

Namely - you're right in that bluebird cuts a lot of edges around scheduling and edge cases the V8 promise does not (for pretty funny historical reasons tbh).

If native promise performance is still an issue for you (we worked on it with the v8 team quite a bit back then) it can be discussed further (feel free to open an issue at the nodejs/performance repo and I promise to engage and ping v8 people).

@talentlessguy
Copy link
Author

I guess this can be closed since Bluebird won't be removed due to perf reasons

@talentlessguy talentlessguy deleted the remove-bluebird branch September 12, 2024 11:46
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.

3 participants