requestAnimationFrame in enable.js not needed

[kofifus]

In cornerstone/src/enable.js we have:

https://github.com/cornerstonejs/cornerstone/blob/master/src/enable.js#L77-#L94

   function draw (timestamp) {
    if (enabledElement.canvas === undefined) {
      return;
    }

    const eventDetails = {
      enabledElement,
      timestamp
    };

    triggerEvent(enabledElement.element, EVENTS.PRE_RENDER, eventDetails);

    if (enabledElement.needsRedraw && hasImageOrLayers(enabledElement)) {
      drawImageSync(enabledElement, enabledElement.invalid);
    }

    requestAnimationFrame(draw);
  }

That is draw keeps being called 60 time a sec

The mechanism of setting needsRedraw to true and waiting for the draw callback to catch it is very inefficient and unnecessary. since needsRedraw is only set to true in two places (invalidate.js and drawImage.js) it would be much more efficient to add a drawAsyncImage like this:

function hasImageOrLayers(enabledElement) {
  return enabledElement.image !== undefined || enabledElement.layers.length > 0;
}

export function drawImageAsync(enabledElement) {
  const eventDetails = {
    enabledElement,
    timestamp: performance.now()
  };

  triggerEvent(enabledElement.element, 'cornerstoneprerender', eventDetails);

  window.setTimeout(() => {
    if (enabledElement.needsRedraw && hasImageOrLayers(enabledElement)) {
      drawImageSync(enabledElement, enabledElement.invalid);
    }
  }, 1000 / 60);
}

and simply call it in these two places which will execute it only once and only when needed. needsRedraw is not needed anymore and can be removed.

I have this change working here without issues and stopped various slowdowns/freezes issues I had.

Please let me know your thoughts.

Thanks!

[swederik]

Thanks for bringing this up. I don't know indeed if we need to maintain the requestAnimationFrame loop at all times. I don't know what downsides this brings, either. I was thinking maybe we should start / stop the loop at some points when the element hasn't been interacted with in a while.

I don't think switching to setTimeout is the answer though, since we want high quality animations, which is exactly what RAF is for.

Do you know what was causing your slowdowns / freezes? How many elements do you have on screen?

One option we have now is renderToCanvas (https://github.com/cornerstonejs/cornerstone/blob/c7beedf1b921ed2deafab89762a421491d4c2baa/src/rendering/renderToCanvas.js#L42) which allows you to just draw an image into a canvas and leave it there without the RAF loop. It is not interactive. We use this for thumbnails, PDF reports, etc...

@jpambrun is our resident performance expert here, I wonder what his thoughts are.

[kofifus]

note my code does not "switch to setTimeout" it only does one setTimeout (which is probably not needed as well and I jusg added it to keep draw async) as opposed to a callback 60 timed a second for no need which is terribly inefficient !

I can't see any other implications as needRedraw is only set to true in two places...

On Friday, May 11, 2018, Erik Ziegler [email protected] wrote:

Thanks for bringing this up. I don't know indeed if we need to maintain the requestAnimationFrame loop at all times. I don't know what downsides this brings, either. I was thinking maybe we should start / stop the loop at some points when the element hasn't been interacted with in a while.

I don't think switching to setTimeout is the answer though, since we want high quality animations, which is exactly what RAF is for.

Do you know what was causing your slowdowns / freezes? How many elements do you have on screen?

One option we have now is renderToCanvas (https://github.com/ cornerstonejs/cornerstone/blob/c7beedf1b921ed2deafab89762a421 491d4c2baa/src/rendering/renderToCanvas.js#L42) which allows you to just draw an image into a canvas and leave it there without the RAF loop. It is not interactive. We use this for thumbnails, PDF reports, etc...

@jpambrun https://github.com/jpambrun is our resident performance expert here, I wonder what his thoughts are.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/cornerstonejs/cornerstone/issues/276#issuecomment-388308327, or mute the thread https://github.com/notifications/unsubscribe-auth/ADvoN0y8JTbyHgevxLj0eV442_Cg5z0-ks5txVX-gaJpZM4T6sPp .

[swederik]

Hi @kofifus

I've added a commit here to test out something like your fix: https://github.com/cornerstonejs/cornerstone/commit/63d9a8e7c4b8929a568b775073adc2b0674dd8f0

Let me know what you think. It definitely makes sense to stop using RAF when we are not animating anything. I'm not sure I like what I've done (attach .draw to enabled element) but I can't think of an alternative at the moment.

[kofifus]

Thx

As far as I can tell the call to draw() in enabled.js is not needed at all, I think it was just there for no reason

I still feel my solution above is preferable: add a drawImageAsync function and remove all needsRedraw and

• add file cornerstone\src\internal\drawImageAsync.js

import triggerEvent from '../triggerEvent.js';
import drawImageSync from './drawImageSync.js';

/**
 * Returns whether or not an Enabled Element has either a currently active image or
 * a non-empty Array of Enabled Element Layers.
 *
 * @param {EnabledElement} enabledElement An Enabled Element
 * @return {Boolean} Whether or not the Enabled Element has an active image or valid set of layers
 * @memberof Enable
 */
function hasImageOrLayers (enabledElement) {
  return enabledElement.image !== undefined || enabledElement.layers.length > 0;
}

/**
 * Draw an image to a given enabled element asynchronously
 *
 * @param {EnabledElement} enabledElement An enabled element to draw into
 * @returns {void}
 * @memberof Internal
 */
export default function drawImageAsync (enabledElement) {
  const eventDetails = {
    enabledElement,
    timestamp: performance.now()
  };

  triggerEvent(enabledElement.element, 'cornerstoneprerender', eventDetails);

  window.setTimeout(() => {
    if (hasImageOrLayers(enabledElement)) {
      drawImageSync(enabledElement, enabledElement.invalid);
    }
  }, 1000 / 60);
}

• in drawImageSync.js remove enabledElement.needsRedraw = false (alternatively drawImageSync can be merged into drawImageAsync which will probably be cleaner)

• remove file requestAnimationFrame.js as it's not used anymore

then

• in enable.js: remove all drawing remove import { drawImageAsync } from './internal/drawImageSync.js'; remove import requestAnimationFrame from './internal/requestAnimationFrame.js'; remove import triggerEvent from './triggerEvent.js'; remove function hasImageOrLayers remove needsRedraw: true, in enabledElement creation remove function draw remove call to draw();

• in invalidate.js: add import drawImageAsync from './drawImageAsync.js'; remove enabledElement.needsRedraw = true; add drawImageAsync(element); at the end

• in drawImage.js: add import drawImageAsync from './drawImageAsync.js'; remove enabledElement.needsRedraw = true; add drawImageAsync(element); at the end

This is what we did here and works fine

[vdnsnkv]

Hi! Is there some progress on this one?

In our web app we need to display a lot of images on one page. With 160 images simultaneously we get 25-30% CPU usage for one page. In Dev Tools I can see that most of it is handling of Animation Frame Fired events. If a user does something on the page, CPU usage goes even higher. For example, simply scrolling the page raises it up to 55-60%.

So I would really like some update. Thanks!

[kofifus]

We have a fix and have been working with it for over a year. I can submit a PR if it helps you but I doubt it will get merged, it seems the cornerstone team is reluctant to change this even though as you say it can totally cripple cornerstone.

[dannyrb]

@kofifus, if you submit a PR, I will take a look. @vdnsnkv, if you submit your example study, I can setup a test for it pre and post merge of the PR.

I believe we've largely shied away from this because we haven't noticed issues in our own applications, and we haven't been provided a cut-and-dry demonstration that this provides significant performance gains.

Most Cornerstone work is done by volunteers or organizations donating resources. Unfortunately, that means we can be strapped for time, and our priorities may differ. When/if contributors can provide:

• A clear example of the issue in a minimal reproduction
• A clear fix/enhancement in a minimal reproduction
• A merge-ready PR

We shift from it being a resource issue to a no-brainer to apply the change.

[vdnsnkv]

@dannyrb I've put together an example of this issue based on cornerstone minimal example. Here it is in one zip

On my laptop this example uses ~200 MB of memory and 15-20% of CPU. CPU usage rises to 20-25% when the page is scrolled.

[dannyrb]

I was caught a bit off-guard when I opened the example and saw 200+ enabled elements. @kofifus, if you have a fix you can PR, I'll test it against @vdnsnkv's example and I'll do a bit of regression testing to make sure everything's working as expected.

If there's a noticeable reduction and low risk, we can merge.

[kofifus]

sure .. it will take me a few days ...

On Thu, Feb 28, 2019 at 8:08 AM Danny Brown [email protected] wrote:

I was caught a bit off-guard when I opened the example and saw 200+ enabled elements. @kofifus https://github.com/kofifus, if you have a fix you can PR, I'll test it against @vdnsnkv https://github.com/vdnsnkv's example and I'll do a bit of regression testing to make sure everything's working as expected.

If there's a noticeable reduction and low risk, we can merge.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cornerstonejs/cornerstone/issues/276#issuecomment-468031249, or mute the thread https://github.com/notifications/unsubscribe-auth/ADvoN3xfxa0m-360K1J6N_usgOtQ__MUks5vRvPggaJpZM4T6sPp .

[kofifus]

Please see https://github.com/cornerstonejs/cornerstone/pull/363

[dannyrb]

Thanks for taking the time to PR the change, @kofifus. I'll post my results here, but may poke at the PR, as well as do some unit/performance testing.

[kofifus]

great, it's pretty involved but as I said been working really well here .. note that I brought the entire lodash throttle function in - since it is in general great it may be good to make it available as a separate module

[dannyrb]

RE: https://github.com/cornerstonejs/cornerstone/pull/363

Thanks again for your work on this @kofifus. It seems there is some room for improvement, but @jpambrun has highlighted more than a few use cases where requestAnimationFrame is incredibly important either for importance or accuracy. The difficulty here is addressing all of the nuances (high time cost) for a solution that is performant for the majority of use cases.

At this point, I think for a PR to be merged, we would need unit test cases that address these points and make sure they are accounted for in whatever new implementation we move forward on.

If @vdnsnkv's use case is common, we could look into providing an escape hatch?

[kofifus]

I worked on this a bit more and have a new PR I will submit in the next few days which does things differently/better:

1. use RAF and keep debouncing turning it off so that its not running if nothing needs to be repainted. This allows at both have the advantages of RAF without the cost of having it running when not needed.

2. Have only one RAF servicing all elements CS is handling

3. I've been working on a multiframe cine player, the existing PlayClip tool uses setTimeout mechanism which is inefficient and does not really make sense when we already have RAF callbacks. I added a mechanism to register for callbacks from the RAF. Tools such as PlayClip can now simply register for RAF callback, switch the image on the element and return without maintaining their own setTimeout mechanisms

I hope to submit this week

[kofifus]

see https://github.com/cornerstonejs/cornerstone/pull/374

[IbrahimCSAE]

Hi Kofifus, I'm also having this problem, and it's killing me, 50% CPU Usage with 900 enabled elements, can you please tell me how to do your fix?

[IbrahimCSAE]

@kofifus

[kofifus]

@IbrahimCSAE since the PR is not merged you will have to patch your code manually using my PR here https://github.com/cornerstonejs/cornerstone/pull/374

[IbrahimCSAE]

@IbrahimCSAE since the PR is not merged you will have to patch your code manually using my PR here #374

Thank you! you are a life saver.