Limit Workers to max. avail. CPUs #12

kurtextrem · 2018-02-10T17:52:27Z

Fixes #10

manjula-dube · 2018-02-10T18:47:54Z

lib/ImageWorker.js

+    this.image = null;
+
+    if (!workerPoolCreated) {
+      createWorkerPool();


One liner ifs can also be made an awesome one

!workerPoolCreated && createWorkerPool();

I forgot a line, that's why I couldn't do the shortcut :)

nitish24p · 2018-02-10T18:50:01Z

lib/ImageWorker.js

+}
+
+/** Marks worker `index` as available. */
+function setFree(index: int) {


change int => number

Thank you, I'm new to flow!

nitish24p · 2018-02-10T18:51:48Z

lib/ImageWorker.js


  state = {
    isLoading: true,
    imgSrc: ''
  }
  constructor(props: ImageWorkerProps) {
    super(props);
-    this.worker.onmessage = (event: Object) => {


What was the purpose of removing the listener from the constructor and moving it to CDM?

I moved everything to the constructor back again. My original idea was that it should only load the image when truly needed, but the constructor is called before CDM.

nitish24p · 2018-02-10T18:55:48Z

Each IMageWorker component will now spawn 4 worker threads. As we've removed the worker.terminate call, will those threads still be active when a componentWillUnMount gets called?

nitish24p · 2018-02-10T18:57:48Z

can you run npm run eslint to see the lint errors and fix those

nitish24p · 2018-02-10T19:16:41Z

lib/ImageWorker.js

+  const blobURL = URL.createObjectURL(
+    new Blob([webWorkerScript], { type: 'application/javascript' })
+  );
+  for (let i = 0; i < window.navigator.hardwareConcurrency || 4; ++i) {


This will go into an infinite loop

when you say

i < window.navigator.hardwareConcurrency || 4

The browser starts evaluating from the left and this will be treated as
(i < window.navigator.hardwareConcurrency) || (4)
even if the value of i goes to say 200, the above will then become
200 < window.navigator.hardwareConcurrency || 4
Which will translate to false || 4 and will always be true

nitish24p · 2018-02-10T19:19:57Z

lib/ImageWorker.js

 class ImageWorker extends Component<ImageWorkerProps, ImageWorkerState> {
  image: HTMLImageElement;
-  worker = new Worker(URL.createObjectURL(


why remove this, where is the worker actually being spawned?

moved to line 42

nitish24p · 2018-02-10T19:23:40Z

lib/ImageWorker.js

@@ -3,15 +3,10 @@
 import React, { Component } from 'react';

 const webWorkerScript = `
+  const handleResponse = () => response.blob();


response is not defined here. When this fn is executed it will break you can change it to

response => response.blob()

kurtextrem · 2018-02-10T20:15:19Z

@nitish24p I fixed the mistakes, sorry for those, I was in a hurry.

nitish24p · 2018-02-10T20:26:32Z

lib/ImageWorker.js

@@ -34,33 +29,71 @@ const wrappedComponent = WrappedComponent => props => {
  return <WrappedComponent {...props} />;
 };

+/** Have we initiated the worker pool already? */
+const workerPoolCreated = false;


I think you will have to change this to a let and reassign it to a true, after the createWorkerPool fn is invoked. Else everytime there will be 4 workers created as workerPoolCreated will always be false

nitish24p · 2018-02-10T20:26:58Z

lib/ImageWorker.js

      this.loadImage(event.data);
+      setFree(workerObj.index);


this should be workerObj.i as per your code

kurtextrem · 2018-02-14T10:11:57Z

There's a big flaw in my current implementation: .onmessage can only exist once it seems. Which means currently ever new instance of ImageWorker replaces the callback function.
~~This means we need something like a Observer pattern.~~

Edit: I've overlooked, that Workers have the .addEventListener interface too, so that's not a problem anymore.

Limit Workers to max. avail. CPUs

5affa3c

manjula-dube reviewed Feb 10, 2018

View reviewed changes

nitish24p reviewed Feb 10, 2018

View reviewed changes

kurtextrem added 2 commits February 10, 2018 21:09

Fix mistakes

0ed703a

Run eslint --fix

bd51edf

nitish24p requested changes Feb 10, 2018

View reviewed changes

kurtextrem added 2 commits February 10, 2018 21:42

Fix another two mistakes

962f748

Style fixes

5ff39d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit Workers to max. avail. CPUs #12

Limit Workers to max. avail. CPUs #12

kurtextrem commented Feb 10, 2018

manjula-dube Feb 10, 2018 •

edited

Loading

kurtextrem Feb 10, 2018

nitish24p Feb 10, 2018

kurtextrem Feb 10, 2018

nitish24p Feb 10, 2018

kurtextrem Feb 10, 2018

nitish24p commented Feb 10, 2018

nitish24p commented Feb 10, 2018

nitish24p Feb 10, 2018

kurtextrem Feb 10, 2018

nitish24p Feb 10, 2018

nitish24p Feb 10, 2018

kurtextrem Feb 10, 2018

nitish24p Feb 10, 2018

kurtextrem Feb 10, 2018

kurtextrem commented Feb 10, 2018

nitish24p Feb 10, 2018

nitish24p Feb 10, 2018

kurtextrem commented Feb 14, 2018 •

edited

Loading

Limit Workers to max. avail. CPUs #12

Are you sure you want to change the base?

Limit Workers to max. avail. CPUs #12

Conversation

kurtextrem commented Feb 10, 2018

manjula-dube Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitish24p commented Feb 10, 2018

nitish24p commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtextrem commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtextrem commented Feb 14, 2018 • edited Loading

manjula-dube Feb 10, 2018 •

edited

Loading

kurtextrem commented Feb 14, 2018 •

edited

Loading