Still seeing decreased performance, please keep CanvasOld #2930
Comments
|
In our tests, we don't have an example where CanvasOld is faster, if you have one please share it with us and we will reopen it immediately (we did act somewhat fast on #2904 right?) In the current architecture we are running the drawing at virtually native speed, anything else from the frame budget is either Reanimated shared value or hermes performance (reconciler, reanimated worklets). But it sounds like we might be missing something just let us know. |
|
I'm very surprised, please let me know if there is anything that I can
easily test on my side.
…On Wed, Jan 29, 2025 at 3:57 PM flowtyone ***@***.***> wrote:
Description
@wcandillon Thank you for all the hard work on trying to fix #2904 but I'm still seeing up to 200% decrease in performance from the old canvas. If this issue isn't fixable for now it's ok, just please keep the CanvasOld implementation around until it is. Feel free to close this issue, I only opened it to ask that.
React Native Skia Version
1.11.1
React Native Version
0.76.5
Using New Architecture
Enabled
Steps to Reproduce
Continuation of #2904
Snack, Code Example, Screenshot, or Link to Repository
Continuation of #2904
—
Reply to this email directly, view it on GitHub or unsubscribe.
You are receiving this email because you were mentioned.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
@wcandillon I'm currently going through updating all of our dependencies including react native itself from version 0.76.5 to 0.77. I'll let you know if that improves anything |
|
@wcandillon after updating all dependencies, performance on the OldCanvas still far supercedes the new Canvas. I can provide you all the details you need as I believe more people will be experiencing this, our use case is not so unique. But I will only be able to share the details with you privately. |
|
yes please provide me an example
…On Fri, Jan 31, 2025 at 10:42 AM flowtyone ***@***.***> wrote:
@wcandillon after updating all dependencies, performance on the OldCanvas still far supercedes the new Canvas. I can provide you all the details you need as I believe more people will be experiencing this, our use case is not so unique. But I will only be able to share the details with you privately.
—
Reply to this email directly, view it on GitHub or unsubscribe.
You are receiving this email because you were mentioned.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
that's a very cool example, would it be ok to add it the example list?
So we can monitor the perf and also see if there are efficiencies to
be found there?
Looking at it now.
…On Sat, Feb 1, 2025 at 5:03 PM flowtyone ***@***.***> wrote:
@wcandillon I managed to put together another standalone example which shows the problem.
import React from "react";
import {Dimensions, View} from "react-native";
import {
Canvas,
Fill,
Group,
Rect,
rect,
rrect,
Skia,
SkMatrix,
useImage,
Image
} from ***@***.***/react-native-skia";
import {runOnJS, useAnimatedReaction, useSharedValue} from "react-native-reanimated";
import {Gesture, GestureDetector} from "react-native-gesture-handler";
const {
width: screenWidth,
height: screenHeight
} = Dimensions.get("window");
const tileProperties = {
size: {
width: 100,
height: 160
},
cornerRadius: 16,
color: "#202020",
padding: 10
}
const multiplier = {
x: tileProperties.size.width + tileProperties.padding,
y: tileProperties.size.height + tileProperties.padding
}
const viewportProperties = {
padding: {
x: screenWidth,
y: screenHeight
},
minScale: 1,
maxScale: 2,
}
interface Viewport {
minX: number;
minY: number;
maxX: number;
maxY: number;
}
interface Point {
x: number;
y: number;
}
const initViewport = {
minX: 0,
minY: 0,
maxX: screenWidth + viewportProperties.padding.x * 2,
maxY: screenHeight + viewportProperties.padding.y * 2,
}
const initPosition = Skia.Point(
-viewportProperties.padding.x,
-viewportProperties.padding.y
)
const initMatrix = () => {
const matrix = Skia.Matrix();
matrix.translate(-viewportProperties.padding.x, -viewportProperties.padding.y);
return matrix;
}
const translateMatrix = (matrix: SkMatrix, x: number, y: number) => {
"worklet";
const m = Skia.Matrix();
m.translate(x, y);
m.concat(matrix);
return m;
};
const getVisibleViewport = (position: Point, scale: number) => {
"worklet";
let minX = -position.x / scale;
let minY = -position.y / scale;
let maxX = minX + screenWidth / scale;
let maxY = minY + screenHeight / scale;
minX -= viewportProperties.padding.x;
minY -= viewportProperties.padding.y;
maxX += viewportProperties.padding.x;
maxY += viewportProperties.padding.y;
minX -= minX % multiplier.x;
minY -= minY % multiplier.y;
maxX += maxX % multiplier.x;
maxY += maxY % multiplier.y;
return {
minX,
minY,
maxX,
maxY
}
}
const getInitialPositions = () => {
const {minX, minY, maxX, maxY} = initViewport;
const positions: Point[] = [];
for (let y = minY; y < maxY; y += multiplier.y) {
for (let x = minX; x < maxX; x += multiplier.x) {
positions.push({x, y});
}
}
return positions;
}
const calculatePositions = (viewport: Viewport) => {
"worklet";
const {minX, minY, maxX, maxY} = viewport;
const positions: Point[] = [];
for (let y = minY; y < maxY; y += multiplier.y) {
for (let x = minX; x < maxX; x += multiplier.x) {
positions.push({x, y});
}
}
return positions;
}
const Tile: React.FC<{ position: { x: number, y: number } }> = ({position}) => {
const iRect = React.useMemo(() =>
rect(position.x, position.y, tileProperties.size.width, tileProperties.size.height), [position])
const clipRect = React.useMemo(() => rrect(
rect(position.x, position.y, tileProperties.size.width, tileProperties.size.height),
tileProperties.cornerRadius,
tileProperties.cornerRadius
), [position])
const image = useImage("https://picsum.photos/200/300")
return (
<Group clip={clipRect}>
<Rect
style="fill"
color={tileProperties.color}
rect={iRect}
/>
<Image
image={image}
rect={iRect}
fit="cover"
/>
</Group>
)
}
const Tiles = () => {
const position = useSharedValue(initPosition);
const prevPosition = useSharedValue(initPosition)
const matrix = useSharedValue<SkMatrix>(initMatrix());
const prevMatrix = useSharedValue(initMatrix());
const tilePositions = useSharedValue(getInitialPositions());
const [tilePositionsState, setTilePositionsState] = React.useState(getInitialPositions());
const viewport = useSharedValue<Viewport>(initViewport);
useAnimatedReaction(
() => tilePositions.value,
(prepared, previous) => {
if (!previous ||
previous.length !== prepared.length ||
(
previous.length > 1 && prepared.length > 1 &&
(previous[0].x !== prepared[0].x || previous[0].y !== prepared[0].y)
)
) {
runOnJS(setTilePositionsState)(prepared);
}
}
,[tilePositions])
const onGestureUpdate = (position: Point) => {
"worklet";
const newViewport = getVisibleViewport(position, viewportProperties.minScale);
viewport.value = newViewport;
if (tilePositions.value.length > 0) {
const topLeft = tilePositions.value[0];
if (
Math.abs(newViewport.minX - topLeft.x) >= viewportProperties.padding.x ||
Math.abs(newViewport.minY - topLeft.y) >= viewportProperties.padding.y
) {
tilePositions.value = calculatePositions(newViewport);
console.log("updating mid gesture")
}
}
}
const onGestureEnd = () => {
"worklet";
// When a gesture ends, we update the positions to match the current viewport
tilePositions.value = calculatePositions(viewport.value);
}
const pan = Gesture.Pan()
.onStart(() => {
prevMatrix.value = matrix.value;
prevPosition.value = Skia.Point(
position.value.x,
position.value.y
);
})
.onChange((e) => {
matrix.value = translateMatrix(
matrix.value,
e.changeX,
e.changeY
);
const newPos = {
x: position.value.x + e.changeX,
y: position.value.y + e.changeY
};
position.value = newPos;
matrix.value = translateMatrix(
prevMatrix.value,
newPos.x - prevPosition.value.x,
newPos.y - prevPosition.value.y
);
onGestureUpdate(position.value)
})
.onEnd(() => {
onGestureEnd();
});
return (
<GestureDetector gesture={pan}>
<View style={{flex: 1}}>
<Canvas style={{flex: 1}}>
<Fill color="black"/>
<Group matrix={matrix}>
{tilePositionsState.map((point) => (
<Tile
key={`${point.x},${point.y}`}
position={point}
/>
))}
</Group>
</Canvas>
</View>
</GestureDetector>
)
}
export default Tiles;
https://github.com/user-attachments/assets/30a51cc5-bc5e-4883-9030-f6e39275d03b
https://github.com/user-attachments/assets/cc974e91-5938-4844-9c72-f957c3d1535c
—
Reply to this email directly, view it on GitHub or unsubscribe.
You are receiving this email because you were mentioned.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
@wcandillon I'd appreciate if you don't put it there, I was not intending to even post it publicly. I only did that so you can try to help us out here. I will remove the code snippet once you have a local copy |
|
of course, sending you an update you will like in a few minutes :) |
|
Ok so this is a very cool and interesting example. And it gives me a lot of food for thoughts on the next improvements to work on. First things first, we had to remove CanvasOld because it contained many bugs and had some stability issues. Its "fatal" design flow is that it treated updates from the JS thread and animation updates equally. That made animations slow. And this is why we find it to be much faster because we only looked at animations. In your case, you only run updates on the JS thread and so indeed in your case things are slower (I didn't do an actual benchmark vs CanvasOld but just by looking at the code, it's very safe to assume). We do want JS updates to be very fast as well, but here the bottleneck really on Hermes speed, we architectured everything to optimize for animation speed where here we virtually run at native speed. Now what do we do from there? I rewrote the example to use only animation values (no JS updates) and then it's crazy fast. For dealing with the images, things get tricky. If you where to rewrite the example without the reconciller, to just draw the scene like a video game loop, you would have the same issue with the image handling where you wouldn't get anything for free. In fact if we can figure out a good image management for this demo, we can definitely rewrite it faster than with CanvasOld (with our without reconciler). I want to give it some more thoughts but let's keep the discussion going on this. |
|
I should mention also that this example did make me find a "quirk" in the new reconciler where a loading image goes from null, to SkImage and is considered a tree/JS update even thought the tree didn't really change, we just need to rerender with the new value. |
|
Can you send the original example privately as well? I think we can really make the example better, I have some ideas, in the end there is no reason that an example like this one would require a full re-render of the JS thread for each animation frame. And that's a bit counter intuitive to what Reanimated does right? |
|
@wcandillon Thank you for the quick reply and as always, great in depth explanation. I can continue the conversation with you privately and even share more details regarding what we're building which would help to make sense of everything. I just haven't figured out how to contact you privately yet. |
|
So my thinking is to figure out the image loading (maybe we need to have a workletRuntime pool to pull/decode images in parallel for instance) and then the images are all available via a shared value anyways and so we can just run it as an animation And you have also figured out the "cleaning out" in your code but I lost that part because I modified the original example too much. |
|
@wcandillon That's a good direction. I messaged you, hopefully we can take it from there |
Description
@wcandillon Thank you for all the hard work on trying to fix #2904 but I'm still seeing up to 200% decrease in performance from the old canvas. If this issue isn't fixable for now it's ok, just please keep the CanvasOld implementation around until it is. Feel free to close this issue, I only opened it to ask that.
React Native Skia Version
1.11.1
React Native Version
0.76.5
Using New Architecture
Steps to Reproduce
Continuation of #2904
Snack, Code Example, Screenshot, or Link to Repository
Continuation of #2904
The text was updated successfully, but these errors were encountered: