Becomes Slow And Unresponsive After Many Messages

[powerqian]

Hi,

I really like this controller overall. However there's one fatal performance issue. I'm not sure whether it's the problem of my code or the controller's.

If there's a large number of messages already, quickly sending and receiving message will become very slow and freeze the App.

I used Instruments to do the profile and saw that in cellForRowAtIndexPath for the controller, it always init a new cell.

What I do when sending and receiving message is basically alloc init a data model, save it to database, add it to data source and call insertRowAtIndexPaths to the tableView. At first everything is fine. But after I keep sending or receiving the messages quickly, eventually the controller will become unresponsive and take a really long time to update the view.

Just wondering why this will happen and the table view not reusing the cell.

[yesidi]

And your device?

[jamieomatthews]

@powerqian You may have to page your view. I have found this to get choppy on my 4s, but only when I get up to 50+messages. You could try paging it to at like 20 messages or so, and when you hit the top, load the next 20, and so on.

The reason cellForRowAtIndexPath returns a new cell for each message is because re-use of cells could have strange effects in our case. Ultimately, you want a new cell for each row.

You can take a look at the implementation in JSMessagesViewController if youd like to experiment with changing that

NSString *CellIdentifier = [NSString stringWithFormat:@"MessageCell_%d_%d_%d_%d_%d", type, hasTimestamp, hasAvatar, hasSubtitle];
JSBubbleMessageCell *cell = (JSBubbleMessageCell *)[tableView dequeueReusableCellWithIdentifier:CellIdentifier];

[powerqian]

Thanks for the reply. My device is iPhone 5. For the testing I just keep sending messages very quickly and soon it will get choppy and after a while, it will block the app. Paging the view would help a little but what if the user what to see the message that is 100+ messages ago when they are send something? It will still block the app after they scroll to the top again and again and again and send or receive message at that moment.

Based on my understanding, the cell will be reused if they have the same look. And I think when scrolling, cells are indeed reused. When I keep sending messages, all the new cell should have met the requirement of reusing (outgoing, all have timestamp, all don't have avatar, all don't have subtitle).

Meanwhile, I'm using the insertRowsAtIndexPaths:withRowAnimation: method of UITableView. In didSendText:, I copy and paste the code of finishSend: without reloadData to achieve the same. I think calling that method won't reload the whole table. It is weird to block the app only if there's a lot of message since the behaviour is the same regardless of the messages number in the table.

[jessesquires]

@powerqian —yeah, @jamieomatthews is right. but, you should try experimenting with the cell identifier and see if you can find any improvements. (if so, please share!)

Also, paging is definitely the way to go. The iOS messages app only displays about 50-60 messages at a time. There are ways to be clever about this. If you are using CoreData as your backend, consider NSFetchedResultsController.

Finally, having said all that... this kind of optimization is definitely something to consider. I'll try my best to look into this after the current iOS UI is a bit more refined. And pull requests are welcome :)

[powerqian]

I know that paging could limit the number of messages, but still, if the user scroll to top and list all the messages and at that moment he sends new messages, it is the same poor performance with not using paging since all the messages are there.

As I described, the issue is not scrolling all the messages. It only block the app when sending or receiving a new message while there are a lot of messages.

I will try to see what I can do.

[powerqian]

Hi, I think I found what cause the issue.

The implementation of heightForRowAtIndexPath of JSMessagesViewController is

- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath
{
    return [(JSBubbleMessageCell *)[self tableView:tableView cellForRowAtIndexPath:indexPath] height];
}

As far as I know, calling cellForRowAtIndexPath: within heightForRowAtIndexPath: will cause problem. There are discussions on StackOverflow about this. In my previous project, I tried to do that, and ended up with a loop: calling cellForRowAtIndexPath: will trigger heightForRowAtIndexPath, and it calls cellForRowAtIndexPath again. But I don't know why here it will not cause the loop.

Specifically, for this case, heightForRowAtIndexPath: is called for every cell if there's a reloadData for table view. And inside that it calls cellForRowAtIndexPath:, which will alloc and init a new JSBubbleMessageCell.

So if there are 100 messages, a single reloadData will alloc and init 100 JSBubbleMessageCell, just in heightForRowAtIndexPath, which will definitely block everything. And if I send and receive message quickly, every time there's a new message, a reloadData will be called (I don't know why it is still get called even though I only use insertRowsAtIndexPaths:forRowAnimation:). The time for alloc and init is slower than the time that messages generates. That is the problem.

I return a constant number in heightForRowAtIndexPath: and everything becomes smooth.

In summary, my suggestion would be having a utility method for JSBubbleMessageCell that calculate the height based on given parameters. Something like

+ (CGFloat)heightForCellHasTimestamp:(BOOL)hasTimestamp avatar:(BOOL)hasAvatar subtitle:(BOOL)hasSubtitle content:(NSString *)content

I have looked some part of those code and it seems quite complicated. I think you should have a clearer mind than me that how to achieve this. However if you don't have time, please let me know :).

[jessesquires]

@powerqian — Yes, this is the issue. However, cells are being dequeued sometimes. The utility method that you suggest is exactly what used to be there, but I dislike static methods like that and refactored it (obviously without thinking enough about performance).

The problem with "putting it back" how it was, is that it requires a lot of refactoring. There's a chain of methods that have to be changed in order for it to work (as you may have noticed). The biggest issue is that the cell height needs to know the bubble view height (via neededHeightForCell), which calculates this based on the string AND font.

So, we'll be losing the modularity of having any kind of font for any bubble, but so be it. (i.e., JSBubbleView method setFont: will be removed and bubbleView.textView.font will not be allowed to be set — however, I can make a UIAppearance font property, which is probably good enough.)

I'm starting this and will push when I can! :) Thanks again for your help!

[vladimirGI]

@jessesquires - maybe it's not related: I have noticed the following bug in the control. my requirement is to highlight first row with a different bubble image.

• (UIImageView )bubbleImageViewWithType:(JSBubbleMessageType)type forRowAtIndexPath:(NSIndexPath )indexPath { if(indexPath.row == 0) { return [JSBubbleImageViewFactory bubbleImageViewForType:type color:[UIColor js_iOS7greenColor]]; }

  return [JSBubbleImageViewFactory bubbleImageViewForType:type color:[UIColor js_iOS7blueColor]]; }

Usually it works fine, however if EU scrolls really fast up & down the last or one before last item is highlighted...This happens only when there few 8+ bubbles... Any ideas? Could it be related to the above explanation

[powerqian]

@jessesquires Actually I have implemented those static methods in the entire calculating height method chain myself and used it as a part of my project. However, I did hardcode the font to be systemFontWithSize:16, which is used by default when initializing the JSBubbleView. I think for my own use, I can change the font to whatever I want, but apparently not good enough for the code on GitHub for every one to use. I hope you can have a work around about this.

Thanks for your great coded control. It saves me a lot of time!

[jessesquires]

@powerqian awesome! the work around here is allowing the font to be set via UIAppearance.

@vladimirGI — I'm not sure, but it sounds like a dequeuing issue.