search

JoshMarler / react-juce

Write cross-platform native apps with React.js and JUCE
https://docs.react-juce.dev
MIT License
764 stars 79 forks source link

Canvas optimisation #212

Closed JoshMarler closed 3 years ago

JoshMarler commented 3 years ago

@nick-thompson,

Tackles https://github.com/nick-thompson/blueprint/issues/133

Opening up a PR just so you can see the work in progress. This all needs a bit of tidying. Main issue is that we would need to return JS function return values from EcmascriptEngine::readVarFromDukStack if we decide to keep this approach.

I think the overall approach should be quite neat though. Will move all the draw commands into cpp and separate Canvas.h into cpp/h pair etc.

JoshMarler commented 3 years ago

Also, what are you thinking? What's left to move this away from WIP status?

Yep, will totally get some A/B profile data on here before merge.

To move from WIP I'm thinking probably moving all those DrawCommand functions to CanvasView.cpp. Sort the juce::Image render and general cleanup. Then I think this can go in. Following that I can tackle the remaining Canvas API functions from our beta list.

nick-thompson commented 3 years ago

Yep, will totally get some A/B profile data on here before merge.

To move from WIP I'm thinking probably moving all those DrawCommand functions to CanvasView.cpp. Sort the juce::Image render and general cleanup. Then I think this can go in. Following that I can tackle the remaining Canvas API functions from our beta list.

Perfect! :+1:

nick-thompson commented 3 years ago

This looks excellent @JoshMarler, really like the shape it's taking. Let me know when you're ready to mark this non-WIP and I'll take another review pass!

JoshMarler commented 3 years ago

@nick-thompson , Should be good for final pass. Functionality is as before. I've not added any additional API calls or fixed the Retina issue in this PR. Once this goes in I'll follow up with those two as separate tasks.

When you're happy with this I'll do the usual npm bump dance.

Will profile on Mac now and get A/B comparison.

JoshMarler commented 3 years ago

So,

Some performance data.

. With this change watching the memory usage monitor in Xcode shows memory sat at a constant (around 56 MB) whilst standalone plugin idle. Prior to this change memory usage steadily climbs from 56Mb whilst idle in standalone plugin. CPU usage similar with both approaches.

. With this change moving slider in standalone plugin shows memory usage fluctuate between 56 -> 80 MB. So still a decent amount of heap churn. Prior to this change shows similar numbers.

Instruments actually shows big allocation hotspots (in terms of frequency) around JUCE drawImageAt inside CanvasView::paint. It's possible we could investigate whether there is anything we can do to improve that. I expect not.

Heaviest stack trace out of juce::CoreGraphicsContext::drawImage. I do wonder if using the paint functions juce::Graphics object would provide a significant performance boost but we've suggested we want to keep this image rendering behaviour for things like scrolling waveforms.

JoshMarler commented 3 years ago

@nick-thompson , I think I actually saw the biggest performance gain in my earlier implementation which didn't render to a juce::Image.

If I remove that image rendering I see 20% CPU reduction in xcode and much less time spent in paint call under Instruments.

JoshMarler commented 3 years ago

@nick-thompson , OK, following our Discord chat and discussions here this should be ready to review now with the stateful prop added as opt-in behaviour to render to a persistent juce::Image instance.