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

disable concurrency in case of libuv/libuv#1459 #65

Closed
wants to merge 2 commits into from

Conversation

Quasic
Copy link
Contributor

@Quasic Quasic commented Nov 16, 2017

When the number of workers/cpus can't be determined, assume 1.
termux/termux-packages#1798
There must be a better workaround?

@Quasic
Copy link
Contributor Author

Quasic commented Nov 16, 2017

It might be better to use trial and error to find the number, instead of using only 1 worker, if that's possible.

See also npm/npm#19170

@rvagg
Copy link
Owner

rvagg commented Nov 16, 2017

How would it do "trial and error"? that'd involve benchmarking I'd guess and that seems pretty out of scope here. Although, if that could be done in a third-party package that we could pull in here to figure it out for us we could consider it.

lib/farm.js Outdated
@@ -2,7 +2,7 @@

const DEFAULT_OPTIONS = {
maxCallsPerWorker : Infinity
, maxConcurrentWorkers : require('os').cpus().length
, maxConcurrentWorkers : (require('os').cpus()||{length:1}).length
Copy link
Owner

Choose a reason for hiding this comment

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

space this out a bit please: ) || { length: 1 })

Sorry. Mobile keyboard was clobbering when I pushed spacebar.
I thought about cleaning it up, but figured it was a bit hacky, anyway.
It should still be understandable,  though.
@Quasic
Copy link
Contributor Author

Quasic commented Nov 16, 2017

Thanks for the replies. I agree that trial and error is neither ideal, nor in scope for implementing here, and I haven't found a way to actually do anything better than set a fallback value, yet. The value could perhaps be higher, though?

@Quasic
Copy link
Contributor Author

Quasic commented Nov 16, 2017

Hmm... Did I somehow manage to break a check while reformatting? I need to clone and use vim...

@rvagg
Copy link
Owner

rvagg commented Nov 16, 2017

flaky tests, don't worry about it, it's my ongoing battle

change landed and worker-farm@1.5.2 published

@rvagg rvagg closed this Nov 16, 2017
Quasic added a commit to Quasic/npm that referenced this pull request Nov 16, 2017
In fixing npm#19170, rvagg/node-worker-farm#65 falls back to basically
disabling multiple workers, so the user may want a switch to override.
@Quasic
Copy link
Contributor Author

Quasic commented Nov 21, 2017

@rvagg, I started a package which has more success finding the number of cpus than os.cpus(). It's my first real node package. l'm not sure where I'm going with it, yet, but if you want a look, https://github.com/Quasic/cpuinfo

@rvagg
Copy link
Owner

rvagg commented Nov 21, 2017

So are /sys/devices/system/cpu, /sys/devices/system/cpu/present and /sys/devices/system/cpu/possible available on Android?

@Quasic
Copy link
Contributor Author

Quasic commented Nov 22, 2017

This package was tested on arm Android 5.1.1 before each push, and the idea to use /sys/devices/system/cpu/present came from an Android 8 user at npm/npm#19170 (comment)

@Quasic Quasic deleted the patch-1 branch November 22, 2017 01:11
@Quasic
Copy link
Contributor Author

Quasic commented Nov 26, 2017

Maybe I should have mentioned @rvagg and perhaps @shannon to make sure Quasic/cpuinfo currently passes npm test on the Android 8 phone... My emulator doesn't want to boot, currently...

Happy Thanksgiving Weekend!

@shannon
Copy link

shannon commented Nov 27, 2017

@Quasic I ran the tests on my device. It's not completely clear whether this is a pass or fail.

https://gist.github.com/shannon/f35d7cf7d931c2594e2b4fe7a9f9d7a0

@Quasic
Copy link
Contributor Author

Quasic commented Nov 28, 2017

@shannon, thanks. If you have 8 virtual CPUs, then it's an overall pass. The EACCES error is the one causing the initial problem. The "Not yet implemented" errors are just where I left off, to mark where to continue. I have one more counting method to script, though it doesn't appear necessary, and I'm not sure it's supported. The mapping method is also unfinished, but here, only the counting methods are needed.

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