Skip to content
This repository has been archived by the owner on Aug 14, 2019. It is now read-only.

Scrolling perfomance on iOS8 #492

Closed
etolstoy opened this issue Sep 16, 2014 · 135 comments
Closed

Scrolling perfomance on iOS8 #492

etolstoy opened this issue Sep 16, 2014 · 135 comments

Comments

@etolstoy
Copy link

I've encountered a strange behaviour on iOS8. When I scroll the collectionView on the device with iOS8 GM, it performs with some lags, while on another device with iOS 7.1 everything is fine.
Is it a well-known issue?

@jessesquires
Copy link
Owner

thanks @igrekde - no one has reported this yet. there are other GM bugs too. hopefully this will resolve itself.

@etolstoy
Copy link
Author

@jessesquires I've tested one of the apps using your library - Parse chat and noticed the same issue.

@rad182
Copy link

rad182 commented Sep 19, 2014

I too experience this performance issue

@madewulf
Copy link

Yep, I saw that too.

@etolstoy
Copy link
Author

BTW, this problem can be seen even in the demo application.

@jessesquires
Copy link
Owner

Hey guys -- thanks for the updates!

We addressed scrolling performance issues awhile back in v5.0.3, but it looks like the issue is back. I'm not 100% sure how to resolve this, so any help is much appreciated!

Also, the release-6.0 branch now has a stable demo. I've made some changes here that should help with scrolling. It feels like it has improved to me, but it still doesn't seem as smooth as iOS 7.0.
Let me know what you think!

@etolstoy
Copy link
Author

@jessesquires I've tested the demo app in release-6 branch, but I still experience the same perfomance issues - maybe a slightly better than in the master.

@madewulf
Copy link

@jessesquires FYI, I just spent a couple hours trying to find where the problem is. All I can say right now is that if you comment out this line:

https://github.com/jessesquires/JSQMessagesViewController/blob/release-6.0/JSQMessagesViewController/Controllers/JSQMessagesViewController.m#L433

everything begins to run more smoothly on my iPhone5. Obviously, this is not an option :-) That said, it seems to rule out problems with rendering the bubbles or the avatars images, at least.

In my test, I simply added 50 outgoing messages in a loop to populate the conversation. The profile shows a lot of layouting activity for the UICollectionView, so my guess is simply that finding a solution for the constraint system of the cell takes too much time, which happens only when the constraints have changed (so, in my test case, when the text has changed).

Hope this helps.

I thought that this SO answer might help too, if you did not see it already: http://stackoverflow.com/questions/18746929/using-auto-layout-in-uitableview-for-dynamic-cell-layouts-variable-row-heights#comment35309097_18746930

For the record, I just spent a few days trying to get a smooth UITableview running with cells layed out using Autolayout, and I never managed to get something satisfactory. I'm guessing that autolayout is still a bit too CPU intensive right now. Doing it "manually" in LayoutSubviews, in the other hands, did give excellent results. Just a data point.

@jessesquires
Copy link
Owner

Thanks so much for the investigation @madewulf ! Very interesting.

What makes this worse is that there are lots of optimizations already in place:

  1. we cache computed bubble sizes for use later
  2. we have a custom invalidation context
  3. we prevent constraint updates unless they have changed

These yielded great scrolling results under iOS 7. So something has changed with iOS 8.

Good to know that is it not the bubble or avatar images. That would have been my first guess.

I wonder if the issue is with data detectors? We should try turning them off.

@LeoNatan
Copy link

I profiled the app, and it seems the issue here is the use of AutoLayout. Once everything related to auto layout is turned off, performance is suddenly very smooth.

You should consider dropping auto layout completely. There is nothing that seems difficult to layout manually, especially in cells, where you already have all the heights (or can calculate them using framework methods.

@jessesquires
Copy link
Owner

Thanks @LeoNatan !

To reiterate, this was not an issue on iOS 7. This was introduced in iOS 8. 😢

Dropping auto-layout is not an option — with size classes and iPhone 6, it should be clear that manual layouts will not suffice. There's a reason Apple has introduced these frameworks — this is the best way to implement adaptive UI. The previous version (v4.0.x) of this library did not use auto-layout — a decision that was the source of countless bugs, limitations, and one reason why v5.0 was a complete re-write. Auto-layout is the reason why this library still "just works" on both iPhone 6 and iPhone 6 Plus in portrait and landscape. (All all other devices 😄 )

However! I think (hope) we can fix this. Don't worry, this is high-priority.

Also — everyone's comments here are a huge help! Thanks so much! Keep them coming. 👍

@LeoNatan
Copy link

I don't really see why in this case. The project is iOS7 and above, so TextKit can give you a very precise estimation of text size. For other things, the API asks the user for precise sizes. You know the container size, you know the layout size and you hear when these sizes change. All this amounts to actually quite a simple (but perhaps a little tedious) layoutSubviews. When the cell is sized correctly by the layout, performing internal layout is just as simple as pinning manually the avatar image and top and bottom to edges, and resizing the text area to fit the rest. For the bottom bar, the Auto Layout can remain, I doubt it has any real performance penalty.

I can help with this if you are interested.

@jessesquires
Copy link
Owner

@LeoNatan -- any/all help is much appreciated!

However, we should continue using auto-layout for the reasons above, but also because the 6.0 release will be finished soon. There are already significant changes in the library for 6.0 — overhauling all of the layout code is too risky this close to release.

For anyone wanting to submit a PR for this issue, please submit to branch release-6.0.

@LeoNatan
Copy link

Release 6 is shaping up to be a very good release. Very cool additions.

The library is not really usable at this point. Devices such as 4S, iPad 2 and mini (non retina) really are not able to push the CPU power needed to layout the scene. I will comment only on iOS8 because I did not test on below, nor is it a consolation. With media attachments, the problem becomes even more so relevant.

In my project, I am still in a technology research phase and POC, so I don't mind the performance hit right now. But I cannot start work with this project with the current performance limitations.

If nothing comes up regarding Auto Layout performance, I will either write a lib of my own or help rewrite this one if timetable permits. Please let me know when possible.

@jessesquires
Copy link
Owner

Hello all:

After some investigation with Instruments, I've found some issues regarding scrolling performance and fixed them. Still not 100% but I think it is much better. Please pull the latest from develop and let me know how it goes.

@jessesquires
Copy link
Owner

Profiling data from Instruments Timer Profiler:

  1. Lots of time spent in collectionView: cellForItemAtIndexPath:
    • 44.8% dequeueReusableCellWithReuseIdentifier: forIndexPath:
    • 16.5% cell.textView.text = messageText;
    • 30.6% cell.cellTopLabel.attributedText = ...
  2. Lots of time spent in prepareForReuse / applyLayoutAttributes:
    • 51.4% self.textViewFrameInsets = customAttributes.textViewFrameInsets;
  3. Lots of time spent in messageBubbleSizeForItemAtIndexPath:
    • 90.1% boundingRectWithSize:

Clearly, there are some additional areas to optimize. Right now, I'm thinking applyLayoutAttributes: could be better.

If anyone else has additional information, please report back here. 👍

@etolstoy
Copy link
Author

etolstoy commented Oct 6, 2014

Hi, @jessesquires. I've tested the latest version of the library - it seems that the scrolling perfomance has improved a little - but it still works badly, especially with a fast scrolling. Anyway, thanks a lot for your effort. I hope, that you'll completely fix the issue.

@LeoNatan
Copy link

LeoNatan commented Oct 6, 2014

For me on 5S it didn't improve sadly.

@jessesquires
Copy link
Owner

@LeoNatan - really? I'm testing mostly on an iPhone 5 and performance is not that bad. It's noticeably not as smooth as before, but it isn't terrible.

jessesquires added a commit that referenced this issue Oct 19, 2014
@jessesquires
Copy link
Owner

Hello all: a bit more progress here. Please checkout the latest on develop and let me know how this is working now.

@etolstoy
Copy link
Author

@jessesquires I've tested the latest version - the perfomance is great!

@LeoNatan
Copy link

Now I'm having expectations ^^

@dereck009
Copy link

I can't say I'm happy with the performance. Scrolling has lags and janks even on a 5S which is troublesome considering I'd like to support devices back to 4S (as iOS 8 does).

Do you plan to improve scrolling after 6.0 is released (I'm guess you won't delay the launch just for this)?

Or is the "ok" performance on a 5S acceptable, cause it's not a priority?

@gblazex
Copy link

gblazex commented Jun 26, 2015

I still have to use my manual layout fork.

@jessesquires did you file a radar about this? I'd like everybody in this thread to dupe the hell out of it! We are many and we are strong :)

Here are 2 videos demonstrating it on a 5S, iOS 8.3 using the latest develop branch (91e74b0):

https://www.dropbox.com/s/2wke30hqxtw6poa/JSQScrolling_iOS_v8_3.mov?dl=1
https://www.dropbox.com/s/cw57yku1qpy0y7m/JSQScrolling_fast_iOS_v8_3.mov?dl=1

And the 5S is a beast in terms of performance.

There is clearly a regression on Apple's part that we have to get them to fix it.

@LeoNatan
Copy link

Not fixed as of iOS 9 beta 2. Scrolling is still terrible.

@gblazex
Copy link

gblazex commented Jun 26, 2015

This project has 5000+ stars, one of the most popular Objective-C repo on Github.

I just got the Chromium team to fix a 5 year old bug by guiding some of my users to their issue tracker. Sometimes you have to shower these big whales with bug reports in order for them to listen.

@gblazex
Copy link

gblazex commented Jun 29, 2015

Guys, I think I solved it. Please test this PR:

#1058

Make sure to disable dataDetectors when testing it so you can see the improvements not affected by them.

They seem to be the biggest reason left to cause occasional jankiness.

@gblazex
Copy link

gblazex commented Jul 1, 2015

Just had a breakthrough with data detectors. Created a PR for TTTLabel and it blows UITextView away.
Creating labels and setting data detectors:

UITextView (1.662s) vs TTT (0.084s)

TTTAttributedLabel/TTTAttributedLabel#548 (comment)

datadetector

@LeoNatan
Copy link

LeoNatan commented Jul 1, 2015

@galambalazs That's a terribly inaccurate and misleading test. What you actually measured is the creation of a text views, rather than data detectors logic cost. Data detectors are actually calculated on a background thread that doesn't even complete its run, if it even gets to run at all in your example, because the text view object is released just after allocation.

@LeoNatan
Copy link

LeoNatan commented Jul 1, 2015

@jessesquires I think this issue needs to be reopened. The scrolling performance is still bad compared to other messaging apps such as WhatsApp, FB Messenger and Telegram.

@gblazex
Copy link

gblazex commented Jul 1, 2015

Haha fair enough, you may think I haven't tested it enough. I've been working on JSQ & TTT for the past few days however. :)

The whole point is moving data detection logic to the background thread! No reason to do it on the main thread. But that was possible with TTTAttributedLabel before.

But in the old TTTLabel allocation of every detector instance ran on the Main thread. One for each label! This is what my PR is about. In the new version the "Label + detector creation" is now much faster. That's what the test screen shows.

I'll upload a JSQ fork in a minute for you to test. :)

@LeoNatan
Copy link

LeoNatan commented Jul 1, 2015

That's not what I said. I said your testing methodology in the above example is wrong. UITextView already performs detector work on a low priority background queue. The test you show above is absolutely wrong. I make no claim that UITextView is more performant than TTTAttributedLabel, and indeed it may be much slower, but the test above is not what proves it.

@gblazex
Copy link

gblazex commented Jul 1, 2015

See my friend, it's just that you happen to be uninformed on that one. If you actually test it, UITextView data detectors do a ton of things on the Main thread. Painfully. Slow.

(now, where the actual regexes run might be another question, but seeing the net effect on the main thread, who cares?)

ddtextkit

(testing 1500 UITextViews with short random text, urls, phone numbers. It's all shown on screen in a scrollview, nothing is deallocated).

@gblazex
Copy link

gblazex commented Jul 1, 2015

Popcorn time! ;)

TTT after new Pull Request: https://www.dropbox.com/s/8bdiwhjepxznbsd/ttt.mov?dl=1
UITextView: https://www.dropbox.com/s/rkldfwert98wtly/uitextview_org.mov?dl=1

Do notice how after painfully loading up all the text views, UITextView takes it's time to do all the data detection (constantly trying to scroll with my fingers, not possible of course, main thread blocked).

@gblazex
Copy link

gblazex commented Jul 1, 2015

here is my quick JSQ fork with the new TTT.

I have a more advanced version in development, but this is just for a quick look for you guys.

https://www.dropbox.com/s/c92957h65r94vhx/JSQ_with_new_TTT.zip?dl=0

@gblazex
Copy link

gblazex commented Jul 1, 2015

Here is the 1500 labels demo in case you wanted to run it yourself.
(up for grabs for anybody else of course :-).
https://www.dropbox.com/s/1spaavjcrd8r5jq/test_1500_labels_uitextview_vs_new_ttt_for_leo.zip?dl=0

There's no menu, so you just change the #define in code to run a UITextView or TTTLabel version.

@appleguy
Copy link

appleguy commented Aug 5, 2015

Hey everyone — really inspiring to see the amount of community collaboration going on here! What a phenomenal example of the iOS OSS community :).

I just wanted to throw it out there that AsyncDisplayKit has added support for automatic layout, and it's working phenomenally well for us at Pinterest. It is not UIKit Autolayout, which is implemented in an opaque way that prevents it from being optimized, but it is Box Model automatic layout that has gained a decisive favor across the industry. Box model is used not only by the whole Web & CSS world, but also React Native, ComponentKit, WatchOS, and is also coming to iOS 9 in part with UIStackView.

From having talked to my friends on the iOS & WatchOS teams, they pointed out that "automatic layout" has gone through three major iterations: autoresizing masks, constraints-based auto layout, and box model-style auto layout. The trend towards box model is clear, both inside and outside of Apple.

Pinterest had been seeing a rapid rise in UIKit Autolayout usage in recent months, and it certainly shows in the app. It's definitely one of the worst performing apps used by >10M people. The cost of the layout actually increases exponentially as new constraints are added, and is not just difficult to optimize, but technically impossible to make significant improvements — there's no way to get the constrained size for text labels so that you could do the work asynchronously. It's a shame, because these problems really do eliminate it as a choice for developers that strive for great performance. The statistical facts of profiling show that using constraints-based auto layout guarantees you will never achieve ideal performance, so Pinterest recently followed suit and banned Autolayout like Facebook and Instagram. We as engineers should be angry that Apple promotes a tool with such detrimental performance for any non-trivial use case.

Remember that Apple's tools cater to the very large number of junior developers in the world. They've always pushed Interface Builder, too — but when I left the iOS team after we developed 5.0, not a single app in the system was built with IB. I can almost guarantee you that a majority of Apple's 1st-party layout code uses thoughtfully-factored manual layout code. Manual layout doesn't need to be tedious, fragile, or repetitive.

All that said, ASLayoutSpec is a really fascinating paradigm that I would encourage folks to check out and give feedback on. Even if you think it's terrible, the community working on it would honestly love the feedback. I'm not sure ASDK is appropriate for JSQMessagesViewController, but it's certainly cool to realize that ASLayoutSpec allows you to create completely asynchronous, multi-core concurrent layout calculations that are anything from 100% automatic to 100% manual. It's a hugely empowering feeling to be able to create a custom layout spec and then apply that anywhere in your app, regardless of the content / elements being positioned. You can take something like the off-the-shelf ASStackLayoutSpec to make a vertical stack of horizontal stacks, and create a flexible grid component without any math. You can add manual layout code at any level, and caching is fully automatic.

Here's the PR that added it, which you can scroll through to see the box model specific classes; the master branch has a refined version.
facebookarchive/AsyncDisplayKit#468

Let me know if I can help answer any questions you have while optimizing, even without ASDK! (If you join the group fb.me/paperf8, send me a direct message on Facebook)

@appleguy
Copy link

appleguy commented Aug 5, 2015

One last thing — this other project isn't nearly as full featured as JSQMessagesViewController, but for anyone interested at poking at performance, it's a reasonable example of an un-optimized ASDK implementation: https://github.com/nguyenhuy/AsyncMessagesViewController . Dialing it in with optional optimization features like layer-backing, precompositing, etc would guarantee 60FPS performance on an iPhone 4, but I'm not sure how close that project gets without having focused on profiling.

@ghazel
Copy link
Contributor

ghazel commented Aug 5, 2015

Tremendous work @appleguy.

I would like to say, though, that for JSQ I am skeptical of AsyncDisplayKit. The native iOS Messages app clearly uses UICollectionView directly, and seems to perform great.

@jessesquires
Copy link
Owner

Thanks for all the thoughts/feedback @appleguy ! 😄

@gblazex
Copy link

gblazex commented Aug 5, 2015

@appleguy great progress with ASDK!

The bottleneck in this project is the slow UITextView rendering & data detecting.
Anyone interested can try out the TTTAttributedLabel fork of this project:
https://www.dropbox.com/s/c92957h65r94vhx/JSQ_with_new_TTT.zip?dl=0

After that switch it's 60 FPS even with auto layout.
(altough I also have a fork that replaces AL with manual layout,
but @jessesquires so far said that he didn't wanna go that route)


And while it's true that Apple seemingly uses UITextView inside ChatKit, after carefully reverse engineering their code (I told you I spent a lot of time with this) they are using private APIs,
one of them being

- (id)initReadonlyAndUnselectableWithFrame:(CGRect)arg1 textContainer:(NSTextContainer)arg2

...which seems to perform waaaay better than a standard UITextView that has selectable and editable properties set after the init. Also async Data detection.

Now, why this is private API, I don't know, but doing my 1500 labels demo with it shows the difference
(altough TTT is still clearly faster).

@dzamani
Copy link

dzamani commented Aug 6, 2015

Hi all,
First I would like to give my thanks to @jessesquires for this project and how you try to keep it up to date, and also to @galambalazs for his fixes for AL (your PR is still not added to develop, IMO it should be).

@galambalazs did you make a new version of JSQ with TTT or is the dropbox link your last try ? Don't you have a fork or a branch for this purpose ? For now, I'm on a fork of a swift-based-chat which use JSQ but I use cocoapod to get the version you did with the AL iOS 8 fixes and I would like to switch to the TTT version properly (so with pods my case). Will you do it or will wait for @jessesquires to update the project ?

@gblazex
Copy link

gblazex commented Aug 6, 2015

@allenn I have a local TTT branch, but it hasn't been pushed to github yet.

There's one main thing that sets TTT apart from UITextView: it supports data detectors, but doesn't provide the default actions that UITextView has (e.g. open Safari for links, Call number for phone numbers, etc.).

I have a nice emulation layer which basically uses a dummy UITextView to simulate the default actions, but I didn't have time to finish it 100%.

Other than that TTT is pretty solid for me.

I'm still waiting for @jessesquires to decide what's the plan for 8.0

If TTT doesn't make it to this repo, I'll publish a 8.0 + TTT fork for sure. :)

@jessesquires
Copy link
Owner

Update:

Hello all. I know things have been moving slowly on this front. Sorry. 😁

The official plan is to move to TTTAttributedLabel(#1337) for the 8.0 release. (#1216)

@gblazex
Copy link

gblazex commented Jan 21, 2016

@jessesquires awesome news!

@thakurdDev
Copy link

In IOS 7 scrolling was working perfect but above IOS 7 scrolling is choopy
and i found a solution for that
Use Below Method in your custom UICollectionview Cell

  • (UICollectionViewLayoutAttributes *)preferredLayoutAttributesFittingAttributes:(UICollectionViewLayoutAttributes *)layoutAttributes
    {
    return layoutAttributes;
    }

@appleios
Copy link

appleios commented Apr 22, 2016

@thakurdDev it will still have this little jitters, even with this "optimization".

Seems that you can't avoid jitters when using autolatout (I've created a simple collection view with bubbles and it jitters when bubbles should be resized from bigger to smaller or vice versa; on low scroll speed as described by @galambalazs). Manual layout is not an easy thing to implement correctly, but smooth scroll deserves the efforts.

@vishnuag05
Copy link

Seriously autolayout sucks , I always wondered whether I am doing something wrong but seems like its a universal problem

@kuyazee
Copy link

kuyazee commented Jun 17, 2016

I'm also running on iphone5s and it lags so much when I scroll up or down

@ippily
Copy link

ippily commented Jul 14, 2016

Has there been any updates on this? On iPhone 5 it is seriously laggy and iPhone 6 it is slightly better

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests