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

Use fast-write-atomic instead of write-file-atomic #21

Merged
merged 1 commit into from
Jan 24, 2019

Conversation

mcollina
Copy link
Contributor

@mcollina mcollina commented Dec 21, 2018

fast-write-atomic is 10-20% faster depending on the operating system,
mainly because it does not use fs.realpath() and so many promises.

This needs to be tested, especially on Windows.

I'm happy to move fast-write-atomic to the ipfs org if you want, I've put it on my profile for convenience of adding Travis etc.

See ipfs/js-ipfs#1785 for more details.

@achingbrain
Copy link
Member

The commit message needs to be changed to be in line with the PL guidelines

Also pre 1.0.0 deps need to be specified with the ~ character instead of ^, guidelines, again.

Once that's done CI should be happier.

@achingbrain achingbrain requested review from pgte and removed request for dignifiedquire January 3, 2019 11:55
Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

LGTM - mainly looked at what fast-write-atomic does under the hood, and that seems to match the minimal thing that we need to do to, to make things work.

@mcollina mcollina force-pushed the use-fast-write-atomic branch from 8041824 to 523a9c6 Compare January 4, 2019 12:52
@mcollina
Copy link
Contributor Author

mcollina commented Jan 4, 2019

@achingbrain done.

@alanshaw
Copy link
Member

alanshaw commented Jan 4, 2019

One thing that write-file-atomic does that fast-write-atomic doesn't is it will queue up multiple writes to the same file and execute them in order. With fast-write-atomic there's no ordering like this.

@mcollina
Copy link
Contributor Author

mcollina commented Jan 4, 2019

One thing that write-file-atomic does that fast-write-atomic doesn't is it will queue up multiple writes to the same file and execute them in order. With fast-write-atomic there's no ordering like this.

That was not part of the algorithm of the Go implementation.

@achingbrain
Copy link
Member

@achingbrain done.

CI says:

⧗   input: 

src: use fast-write-atomic instead of write-file-atomic

fast-write-atomic is 10-20% faster depending on the operating system,
mainly because it does not use fs.realpath() and so many promises.

✖   type must be one of [build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test] [type-enum]

Change src: to perf:...

fast-write-atomic is 10-20% faster depending on the operating system,
mainly because it does not use fs.realpath() and so many promises.
@mcollina mcollina force-pushed the use-fast-write-atomic branch from 523a9c6 to da4de26 Compare January 4, 2019 15:54
@mcollina
Copy link
Contributor Author

mcollina commented Jan 4, 2019

should be done now :D

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM.

There's recursion without process.nextTick in a couple of places in fast-write-atomic Do you think there's a chance of stack overflows on busy systems?

@mcollina
Copy link
Contributor Author

mcollina commented Jan 4, 2019

There's recursion without process.nextTick in a couple of places in fast-write-atomic Do you think there's a chance of stack overflows on busy systems?

Can you send links? I think everything is taken care of.

@mcollina
Copy link
Contributor Author

mcollina commented Jan 4, 2019

Those cannot overflow because open and write calls the callback asynchronously by design.

@alanshaw
Copy link
Member

@pgte can we get your review, merge and release please 🙏

@alanshaw
Copy link
Member

ping @pgte

@pgte pgte merged commit bf677b4 into master Jan 24, 2019
@ghost ghost removed the status/in-progress In progress label Jan 24, 2019
@pgte pgte deleted the use-fast-write-atomic branch January 24, 2019 19:38
@pgte
Copy link
Contributor

pgte commented Jan 24, 2019

Landed on v0.8.0.

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.

5 participants