Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO NOT MERGE] NFT Grid Performance Changes #13295

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Jan 31, 2025

Description

We had some issues surrounding the NFT grid on Android.

VirtualizedList: You have a large list that is slow to update - make sure your renderItem function renders components that follow React performance best practices like PureComponent, shouldComponentUpdate, etc. {"contentLength": 2984.727294921875, "dt": 24841, "prevDt": 1225}

java.lang.RuntimeException: Canvas: trying to draw too large(1435879448bytes) bitmap.
android.graphics.RecordingCanvas.throwIfCannotDraw(RecordingCanvas.java:266)
android.graphics.BaseRecordingCanvas.drawBitmap(BaseRecordingCanvas.java:75)
com.horcrux.svg.SvgView.onDraw(SvgView.java:133)
android.view.View.draw(View.java:24842)
android.view.View.updateDisplayListIfDirty(View.java:23692)
android.view.View.draw(View.java:24573)

java.lang.OutOfMemoryError
android.graphics.Bitmap.nativeCreate(Native Method)
android.graphics.Bitmap.createBitmap(Bitmap.java:1229)
android.graphics.Bitmap.createBitmap(Bitmap.java:1186)
android.graphics.Bitmap.createBitmap(Bitmap.java:1134)
android.graphics.Bitmap.createBitmap(Bitmap.java:1093)
com.horcrux.svg.SvgView.drawOutput(SvgView.java:266)
com.horcrux.svg.SvgView.onDraw(SvgView.java:130)
android.view.View.draw(View.java:24842)

This fix should help alleviate some of these issues.

  1. Refactored some props to prevent unnecessary re-renders
  2. Removed the useSvgUriViewBox as this made an expensive call to fetch an SVG. Instead rely on the existing Image.getSize for dimensions.
  • The useSvgUriViewBox never had any bounding which caused it to render large viewboxes.

There are other things we can do (throttling renders, flashlist) but lets see if we can get the list scrolling without crashing first.

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-notifications DEPRECATED: Use "team-assets" instead label Jan 31, 2025
@Prithpal-Sooriya Prithpal-Sooriya added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jan 31, 2025
Copy link
Contributor

github-actions bot commented Jan 31, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: d65ad55
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/97070df2-fc81-42aa-9a01-3288c581d7fe

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

some SVG images were unbounded allowing them to grow to their full size. This is an issue for android since it can also cause the app to crash if trying to render a massive canvas,
This uses expo image which gives us a nice perf win for collectible media (nfts)
@Prithpal-Sooriya Prithpal-Sooriya changed the title feat: improve nft grid performance [DO NOT MERGE] NFT Grid Performance Changes Feb 5, 2025
since we are instead replacing this component to use the expo-image version
Comment on lines +17 to +21
collectibleIconContainer: {
width: '100%',
aspectRatio: 1,
marginBottom: 10,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some images do not have a size, and collapse to 0. This ensures that we at least give the container a size, so the image can fill it.

flexGrow: 0,
flexShrink: 0,
flexBasis: 97,
aspectRatio: 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double check this I don't think it is necessary.

import Identicon from '../../UI/Identicon';
import ComponentErrorBoundary from '../../UI/ComponentErrorBoundary';

interface Props {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to keep the interface similar-ish to RemoteImage.
Honestly that interface is a mess, so I am open to updating this interface to make it a little cleaner and easier to understand.

style,
});

const FadeInContainer = useCallback(
Copy link
Contributor Author

@Prithpal-Sooriya Prithpal-Sooriya Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These containers act similar to the conditionals in RemoteImage, but just keeps the conditionals contained here.

Gives us a cleaner return JSX output.

@Prithpal-Sooriya Prithpal-Sooriya added the DO-NOT-MERGE Pull requests that should not be merged label Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE Pull requests that should not be merged Run Smoke E2E Triggers smoke e2e on Bitrise team-notifications DEPRECATED: Use "team-assets" instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants