cloudinary / cloudinary-react

React components that utilize Cloudinary functionality
MIT License
502 stars 219 forks source link

Images blocking rendering #20

Closed slackday closed 4 years ago

slackday commented 7 years ago

I have found another issue when testing these components some more. Each <Image> <Transformation> component is blocking the rendering of whole app.

Each image add about 120-300ms when testing. This means that having around 16 images on a single page adds between 3-5 seconds of page loading.

I can't find the exact cause but from a quick look it seems to be related to calculating the imagesize and URL.

screen shot 2017-05-25 at 14 41 07

And this happens every time the components are loaded.

For example if I have 16 images on start page

Start page (+5sec blocked loading) -> Click on subpage (loads fast no images) -> Click back to start page (16 images already downloaded but page freezes for 5 sec anyway).

This makes cloudinary-react unusable at the moment.

tocker commented 7 years ago

@slackday thanks for reporting this! To speed things up, can you please share your page (e.g. in codepen.io)? If you want to keep the code private you can open a support ticket and submit the code there.

tfreeborough commented 7 years ago

+1

When rendering any component that contains the cloudinary transforms I get a similar performance deficit.

tfreeborough commented 7 years ago

This may not be much consolation but a really hacky work-around for now is to have a state that stops the components rendering and in a componentDidMount function update that state in a set timeout.

  constructor(props, context) {
    super(props, context);
    this.handleSetMounted = this.handleSetMounted.bind(this);

    this.state = {
      hasMounted: false
    }
  }

  handleSetMounted()
  {
    this.setState({
      hasMounted: true
    });
  }

  componentDidMount()
  {
    setTimeout(() => {
      this.handleSetMounted();
    },500);
  }

  render() {

    return (
      <div id="Component">
       {
            this.state.hasMounted
            ?
            <CloudinaryContext cloudName="CLOUD NAME">
              <Image publicId="PUBLIC_ID">
                <Transformation width="1000" height="1000" radius="max" crop="crop"/>
                <Transformation width="128" height="128" crop="scale"/>
              </Image>
            </CloudinaryContext>
            :
            false
        }
      </div>
    )
  }

Something like this works fine, but its a shame that it has to be done this way, if I get a chance i'll take a look at the codebase and see if I can make a PR to fix it.

slackday commented 7 years ago

Sorry I have not had time to try this more and provide a code example.

Interesting solution @angrytoad . Did you try using React.PureComponent for rendering instead before hacking the state?

I solved this temporarily in my project by using https://www.npmjs.com/package/react-cloudinary instead.

tfreeborough commented 7 years ago

Hi @slackday,

I'm afraid I have not tried it with React.PureComponent, it would be interesting to give this a shot in a bare-bones example to see if it solves the issues.

I wasn't aware there was another react cloudinary library so I'll have to see if this also solves the issue for me.

Cheers,

slackday commented 7 years ago

Yes it's a bit confusing with so similar names. 😄

If you decide to try it here's what I did in my component

import { Image, Transformation } from 'cloudinary-react';
import { cloudinaryConfig, CloudinaryImage } from 'react-cloudinary';
        {useCloudinaryReact ? (
          <Image
            alt=""
            cloudName={cloudinaryCloudName}
            publicId={image.public_id}
            responsive
            secure
          >
            <Transformation
              dpr="auto"
              crop="fill"
              gravity="face:center"
              placeholder="blank"
              width="auto"
            />
          </Image>
        ) : (
          <CloudinaryImage
            className="img-responsive"
            publicId={image.public_id}
            options={{
              width: imageWidth,
              height: imageHeight,
              ...defaultImageSettings,
            }}
          />
        )}

And I set const useCloudinaryReact = false; until this issue is fixed or I have time to fix it 😁

eitanp461 commented 7 years ago

@angrytoad @slackday I have created a Glitch that displays 3 different image components and transformations for 9 public ids, see https://glitch.com/edit/#!/cloudinary-react-sdk I didn't see any performance problem there.

Please add more details regarding your setup. If you can recreate the issue by remixing the Glitch even better.

Thanks

leibole commented 7 years ago

+1

Really big performance degradation when using the Image component with Transformation.

roeeba commented 7 years ago

Hi, @leibole. As @eitanp461 mentioned, we need some more information regarding the setup, in order to better understand the issue. Could you please share with us this information?

liamjohnston commented 6 years ago

Oh man, the HOURS I spent tracking down this bug. For me it was quite severe, causing most of my page to not render at all. But weirdly, only on mobile, making it extra tricky to debug.

The fix I went with was just referencing the image and transforming it by URL, not using cloudinary-react at all, i.e.:

<img src={`${IMG_PATH}c_scale,h_800,w_800,f_auto/v1/${details.artworkId}`} ... />

(may not work for all use cases)

tfreeborough commented 6 years ago

So I ended up having to come back to this problem after over 6 months and I have some thoughts.

  1. Firstly, it looks like if you remove the Cloudinary Context AND Remove the individual Transformation components and instead apply transformations to only the image component then this solves the problem.
  2. If you need more than one transformation, I'm not currently aware of a way to achieve this using only the Image component.

Unfortunately, I don't know enough about this library to propose a fix, but looking at how its rendered in the console tells me the problem lies with putting children inside of the <Image /> component, with no children it takes on average 50ms to apply a transformation and return the image to the DOM. Even with 1 Transformation inside that jumps up to http://puu.sh/zHoLJ/9f2b6d34d5.png around 3s, not including another 3s where no network activity is being performed http://puu.sh/zHoOj/6a7b0adc94.png.

Hopefully this helps someone get closer to tracking down the issue.

eitanp461 commented 6 years ago

@angrytoad this should have been fixed by #39, and published in version 1.0.6. Which version of the SDK are you using?

nryoung commented 6 years ago

@eitanp461

@angrytoad this should have been fixed by #39, and published in version 1.0.6. Which version of the SDK are you using?

We are using this version and we are still seeing ~470ms render delays per image that uses this component.

I am not sure why this component would block rendering when fetching the image?

eitanp461 commented 6 years ago

@nryoung can you reproduce this problem in an online sample we can access?

nryoung commented 6 years ago

@eitanp461 I created a sample app here: https://nifty-sword.glitch.me

It is especially noticeable when the images are not cached in the browser, it seems to delay rendering until all of the images are loaded.

strausr commented 6 years ago

@nryoung What we're seeing is normal creation time for on-the-fly transformations. Another option will be to generate the transformations eagerly https://cloudinary.com/documentation/upload_images#eager_transformations

nryoung commented 6 years ago

@strausr I unfortunately couldn't replicate it exactly in the example app, but in a real application with tons of DOM elements the transformations slow down significantly, eg. ~500ms per transformation.

Besides this being the normal creation time for on-the-fly transformations, you will notice even in the example app it blocks rendering of any of the DOM elements until the all of the transformations are complete. This shouldn't block rendering of non transformation elements.

strausr commented 6 years ago

@nryoung Unfourtnantly, we were unable to see any blockage of the DOM elements. That being said can you please share with us your yarn.lock or package-lock.json? Also, can you send us your profiling data from Chrome?

roeeba commented 5 years ago

Closing this issue due to the time elapsed. Please feel free to either re-open the issue, contact our support at http://support.cloudinary.com or create a new ticket if you have any additional issues.

jimmiebtlr commented 5 years ago

This is still a major issue

image

jimmiebtlr commented 5 years ago

From package-lock, on the most recent version.

image

jimmiebtlr commented 5 years ago

Getting rid of transform noticeably speeds up the app.

jimmiebtlr commented 5 years ago

Looks like #82 is the same issue.

wallawe commented 5 years ago

Just spent hours trying to figure this out - my modal with 17 images takes 6 seconds to open (they're very small images too)...

When I switched over to using the direct URL everything worked fine.

Before: image

Profiler makes it look like the GPU got completely toasted Screen Capture on 2019-04-03 at 12-22-17 :

After switching over to this everything was fine: image

tocker commented 5 years ago

@wallawe can you try the Image component with a transformation attribute instead of a tag?

let tr= {
  quality: "auto", 
  height: 56,
  width: 80,
  crop: "thumb",
  fetchFormat: "auto"
};

// ...

heroImages.map(img=>
  <Image publicId={img} cloudName="hidden" transformation={tr} key={img}></Image>
)

Let us know if it made a difference.

wallawe commented 5 years ago

@tocker Yep, that solved the problem as well. Thanks for your reply.

edwardsilhol commented 5 years ago

Hey guys, I had to hunt for this for hours, maybe update the Cloudinary React documentation with this syntax ? Thanks for the solution !!!!

tocker commented 5 years ago

We need to figure out why this tag is misbehaving. Otherwise, I'm considering to drop the transformation tag altogether.

slackday commented 5 years ago

Hey returning to this two years later I believe I've found the cause of this issue. However I'm not so experienced debugging libraries/node_modules. Anyone know a good way how I can run this library and do some modifications to try out?

tocker commented 5 years ago

@slackday You can fork this repository, make your changes and run the tests. You can play with the components in the storybook:

npm run build-storybook
npm run storybook

The story at stories/index.js is linked to the sources so any change you make will affect the result. Modify the storybook or create your own storybook to test the transformation component.

slackday commented 5 years ago

@tocker Thanks I will give it a try. Sounds like a good opportunity to learn some new tools.

If you want to move along on your own hand with resolving this issue I found that it seems to be related to the Image component and more specifically calling handleResize on componentDidMount.

https://github.com/cloudinary/cloudinary-react/blob/master/src/components/Image/Image.js#L79

Issue goes away if I comment out that line. Also images renders same as with the handleResize running.

In my opinion the resize can be best handled by each App individually. But I don't know the philosophy behind this function. But it seems difficult for cloudinary-react to make assumptions on how to handle resize.

Also looking at the function it is unclear what is going on. These are the lines I wanted to test more and figure out https://github.com/cloudinary/cloudinary-react/blob/master/src/components/Image/Image.js#L67-L69

tocker commented 5 years ago

@slackday @wallawe @edwardsilhol @jimmiebtlr @angrytoad @nryoung Version 1.1.1 was published today. It should fix the problem.

I'd appreciate your feedback!

roeeba commented 4 years ago

Closing this issue due to the time elapsed. Please feel free to either re-open the issue, contact our support at http://support.cloudinary.com or create a new ticket if you have any additional issues.