fix ulimit issues on large projects

[brysgo]

we spent several days troubleshooting the ulimit issues we were running into on wsl and this was the only way we managed to fix it

[mvayngrib]

@brysgo thanks for the PR, but using readFileSync is a bit drastic. There are plenty of ways to limit concurrency in Node.js, e.g. see the libraries p-limit, p-queue, throat, make-concurrent etc. If you want to refactor to use one of those and pass in an optional --concurrency flag, i'd be happy to merge

[brysgo]

I am curious what the performance gain is of opening all these files at once, we have quite a large project and I didn't notice much of a difference in speed.

[brysgo]

That being said, I opened this PR more as a discussion piece than anything else.

[mvayngrib]

we have quite a large project and I didn't notice much of a difference in speed.

fair enough!

That being said, I opened this PR more as a discussion piece than anything else.

I'm happy to leave this open

[brysgo]

What if we made an assumption that people will have a max of 16 cores and only hold on to 16 file handles at a time?

[brysgo]

although, now that I'm thinking about it, node won't work across cores anyway, so why would it be faster to to load the files async?

[mvayngrib]

node won't work across cores anyway

fs.readFileSync is synchronous, so no other JS can be executed in parallel (in the same process). The classic example is that using fs.readFileSync in an http server's request handler slows down request processing for all other users, while using fs.readFile lets the next request be processed immediately, in parallel to the first. So an individual fs.readFileSync is likely faster than an individual fs.readFile, but it's at a cost

however, i read a bit more about fs.readFile vs fs.readFileSync performance outside of an http-server-like environment, and it seems there really is not much to be gained perf-wise by using fs.readFile, at least for relatively small files like package.json!

in your PR, you've only changed some of the readFile -> readFileSync, and there's also writeFile -> writeFileSync to be done. Are you up for adapting the rest?

[brysgo]

sure!

[brysgo]

On second thought, it may be a bit drastic to convert them all. The reason we saw the failures around the package.json is because they were so small it was opening all of them. This means to me that all the other places have surrounding code that runs slow enough to moderate the number of file handles open naturally. In an effort to not mess with something that works, maybe we can try doing it incrementally.

[brysgo]

okay I changed it to writeFileSync, but I haven't tested it

[brysgo]

Okay, tested it out on: https://github.com/mvayngrib/adexample and it appears to be doing what is supposed to. $ diff node_modules/levelup/package.json ~/workspace/adexample/node_modules/levelup/package.json

65c65,73
<     "semver": false
---
>     "semver": false,
>     "path": "path-browserify",
>     "fs": "react-native-level-fs",
>     "_stream_transform": "readable-stream/transform",
>     "_stream_readable": "readable-stream/readable",
>     "_stream_writable": "readable-stream/writable",
>     "_stream_duplex": "readable-stream/duplex",
>     "_stream_passthrough": "readable-stream/passthrough",
>     "stream": "stream-browserify"
72,73c80,94
<   "license": "MIT"
< }
---
>   "license": "MIT",
>   "react-native": {
>     "leveldown": false,
>     "leveldown/package": false,
>     "semver": false,
>     "path": "path-browserify",
>     "fs": "react-native-level-fs",
>     "_stream_transform": "readable-stream/transform",
>     "_stream_readable": "readable-stream/readable",
>     "_stream_writable": "readable-stream/writable",
>     "_stream_duplex": "readable-stream/duplex",
>     "_stream_passthrough": "readable-stream/passthrough",
>     "stream": "stream-browserify"
>   }
> }

[mvayngrib]

@brysgo thanks! i'll try to give it a test later today