I just ran a benchmarking test as the one used in Ruby, but as I expected: The synchronous solution (~170msec) is >10x faster than Workers (~2300msec), and about 2x faster than Promises (400msec).
Edit: 20 Arrays of 100,000 characters each.
Try with fewer but longer strings?
That’s modern JS for you, optimized AF.
Maybe if the test suite was really large, like one billion characters, or one million arrays, it may make a difference?
Also, I don’t think I explained well my point about invalidating solutions. What I meant was that if we change the tests to expect an asynchronous function, you can no longer pass with a synchronous one, you’d have to always wrap your output in a promise, even if you’re executing code synchronously inside.
Maybe it wouldn’t matter if we tell student to just use an async function, but that’d be limiting them, in a sense
If your machine allows for it, it may be faster with a large dataset and many workers, because I suspect there wouldn’t be a measurable difference with one worker since you’re just delegating all the work to another thread, that essentially executes the counting synchronously.
Technically, one worker thread and one main thread is still parallel, so it satisfies the goal, but it’s the same in term of actual work being done.
The discussion is about:
- the current tests narrow students possibilites down to only synchronous solutions
- the example solution is a synchronous solution that works by accident
- the instructions, that made @ErikSchierboom accept the exercise at all, is elaborating on something, that cannot be done with existing tests
- it is easy to widen the spectrum of possible solutions
- it is easy to provide a concurrent example that works by design
- it is possible to fulfill the promises made by the exercise title and the instructions
If we just agree to disagree leaves the exercise in an artificially narrowed state and no concurrent or parallel solution will ever be submitted. But, I believe we don’t have to enforce concurrent or parallel solutions. This is, where I don’t follow @iHiD.
What I showed in my merge request is, that your synchronous solution passes the asynchronous tests with no changes at all.
OK, so just to summarise. I feel like our general positions are:
- @mk-mxp is arguing for having the exercise solvable both sequentially and using async.
- @themetar is arguing for having the exercise only solvable using async (Based on “Well, to put it bluntly, you shouldn’t have done that.” to allowing a solution that doesn’t implement concurrency to pass the tests).
- @Cool-Katt is happy for @mk-mxp’s proposal as long as existing solutions still work.
Am I clear on everyone’s positions before we continue discussing further? (If you could 
this if so, or clarify in a post if not, that would be ideal 
)
3 Likes
I would be happy with @mk-mxp’s proposed solution, if we can all agree it makes sense.
or
I’d also be happy to change the tests to expect async solutions only, if we can agree that it’s better to force students into using promises, instead of allowing them to cheat with synchronous solutions.
In either case, I’d also really love to be able to test for use of Worker threads if this is possible at all, but I couldn’t figure it out so idk if it is.
When I commented on the PR, I didn’t actually look at the contents (apologies @Cool-Katt, you generally have great PRs so I didn’t think about it, I just wanted to comment on the non-paralelistic nature of JS). My comment there was that concurrency is possible but parallelism is not, without workers, and it should be implemented as such.
Looking at the code now I believe the following changes must be made, which is pretty much in line with what @themetar said (albeit a bit strongly):
- Expect something async, either via callbacks or promises; this means that the tests must be promise based. You can check the
promisesexercise to see how to do that, because the test-runner does support it. - Remove the
resultsobject passing. If the functions are pure then the promises “make more sense”, hollistically.
I also saw that @mk-mxp did a PR. I have not explored it yet, but given their contribution in this thread I actually think that it will contain exactly what I said above.
A worker solution using node workers will pass the same tests a promise based solution would, so perhaps proof.ci.js can be that, to show you can technically do worker based solutions. I also recommend we update the documentation of this exercise to hint at workers.
1 Like
Thank you.
I have updated the tests to expect an async function so we can limit synchronous solutions.
I have also reworked the proof.ci.js to use worker threads as a proof that this can be solved with real parallel execution, and I’ve updated the docs to better reflect the proper way of doing it.
@mk-mxp and @themetar, you’re both listed as contributors for this exercise because you’ve contributed quite a bit to this discussion so credit where credit is due. Thanks for pointing out the flaws, even if we did get into a heated debate about it.
Let me know if there’s anything more to be done - @SleeplessByte and the other maintainers.
5 Likes
I think that is indeed the best way to go about this. Thanks!
2 Likes