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

Improve table view performance #40

Merged
merged 14 commits into from Jan 29, 2016
Merged

Conversation

polqf
Copy link
Contributor

@polqf polqf commented Jan 6, 2016

🎩 What? Why?

As pointed out in #27 , when using ActiveLabel inside a cell, the whole customisation process that is behind any change on it, makes the table to drop a lot of frames.
The customisation ActiveLabel is doing on every change is expensive, so the intention is to try to reduce the cost of every change.

⚠️ Disclaimer: This PR has a huge refactor, so has to be treated carefully before merging

Things covered:

  • Changed towards a tableView Demo
    This change may not be the better option as a the example project, so considering changing it back to the current example.. 🤔

    • Revert to previous Example
  • Rethought way to extract elements from the label text:
    Previously the way ActiveLabel was searching for ActiveElements was by iterating through all the words the label had, and then passing a Regex individually to each one of them to see if they were ActiveElements or not.
    Now the Regex Parser is passing the Regex to the whole text at once. And extracting elements based in specific Regex for each ActiveType

    • Added RegexParser whose only responsibility is to manage Regex related things
    • Added ActiveBuilder. Taking advantage of the RegexParser creates ActiveElements
    • Removed top level functions that were passing a regex to each work
  • Changed Way to Test
    Previously, the way ActiveElement was checking things was by checking if an String was satisfying the requirements to be an specific ActiveType. This way of testing has been broken with the refactor, so the new way the tests are working now is by passing a text to an ActiveLabel and expect that it to contain X amount of active elements, and check the string and type of it.

    • Change Tests
    • Add more Tests
  • Perform only necessary things when changing variables
    At the moment, each time the user sets a property (overridden & custom) we are executing updateTextStorage(). This is expensive, and could be a bottleneck.

    • Avoid calling updateTextStorage() if possible when changing a prop
  • Regex is now detecting # and @ in the middle of words
    That should not occur ⛔

  • Move urlDetector to use regex:
    I've been looking for a correct URL Regex, and I've came across with this page: https://mathiasbynens.be/demo/url-regex. The option I've chosen is the one purposed by @krijnhoetmer, and I've tweaked it a bit

    • Use regex for the urlDetector

    ⚠️ Dropped support for detecting urls like google.com.
    Maintaining support for old school www.google.com

📈 DATA

This table reflects the necessary time to customise a cell changing text and textAlignment when calling cellForRow:

Necessary time (s) Before After
First iteration 0.015 - 0.020 0.002 - 0.003
Going async 0.015 - 0.020 0.0005 - 0.0007

In some cases 10x less time 😱

😮 IMPORTANT:

Even that this PR reduces the label reuse cost, it is not completely solving the issue

👻 GIF

@polqf polqf added the WIP label Jan 6, 2016
@polqf polqf force-pushed the feature/improveTableViewPerformance branch from d6f5a66 to b66b029 Compare January 8, 2016 21:02
@polqf polqf added enhancement and removed WIP labels Jan 8, 2016
@polqf
Copy link
Contributor Author

polqf commented Jan 8, 2016

@schickling Any ideas on how to reduce more the reuse time?

@polqf
Copy link
Contributor Author

polqf commented Jan 8, 2016

After profiling a little bit more, here we have 2 really expensive lines:

  • return urlDetector.matchesInString(text, options: [], range: range) - RegexParser.swift - line 28
  • textStorage.setAttributedString(mutAttrString) - ActiveLabel.swift - line 189

Seems like NSDataDetector is slower than a custom Regex (2x the time)... Maybe we should move to a regex for the URLs?

@schickling
Copy link
Contributor

@poolqf unfortunately I'm very short on time at the moment but moving to Regex sounds good to me!

@polqf
Copy link
Contributor Author

polqf commented Jan 20, 2016

@schickling

Quick update:

⚠️ Dropped support for detecting urls like google.com.
Maintaining support for old school www.google.com
(Same as what github does)

There is no good REGEX way to achieve google.com detection as far as I know (and tried)

I think this PR should not be merged until you have time to completely review it.
I'll use all this changes in my app, to test if it is breaking changes 😉

@polqf
Copy link
Contributor Author

polqf commented Jan 20, 2016

Issue found for now:

  • urls like pic.twitter.com/XXXX are not highlighted ⛔

@polqf polqf added the WIP label Jan 20, 2016
@polqf
Copy link
Contributor Author

polqf commented Jan 21, 2016

pic.twitter urls fixed. Ready to review @schickling 😉

@polqf polqf removed the WIP label Jan 21, 2016
@polqf polqf force-pushed the feature/improveTableViewPerformance branch from 8b811e3 to 188b7a4 Compare January 22, 2016 21:25
@polqf polqf force-pushed the feature/improveTableViewPerformance branch from 188b7a4 to c4a1449 Compare January 22, 2016 21:35
@polqf
Copy link
Contributor Author

polqf commented Jan 27, 2016

Quick update

Been using this refactor in my personal app, and I did not have any issues related to it 🎉

@schickling
Copy link
Contributor

LGTM 👍

polqf added a commit that referenced this pull request Jan 29, 2016
@polqf polqf merged commit ccbb286 into master Jan 29, 2016
@polqf polqf deleted the feature/improveTableViewPerformance branch January 29, 2016 12:32
@JaviSoto
Copy link

JaviSoto commented Feb 4, 2016

Hey!

I just updated to the latest Pod version which includes this, and I'm getting a consistent crash here:

mutAttrString.setAttributes(attributes, range: element.range)

Which is called asynchronously from updateTextStorage. That dispatch_async seems like asking for trouble (race conditions). In fact, if I remove the dispatch_async and call the method synchronously, the crash doesn't happen.

I was just wondering if you've seen anything like this. Would you like me to create an issue? I tried to look for the root cause but I can't seem to find exactly why this is happening.
This is the exception being thrown:

2016-02-04 11:16:44.206 [78159:5339064] *** Terminating app due to uncaught exception 'NSRangeException', reason: 'NSMutableRLEArray replaceObjectsInRange:withObject:length:: Out of bounds'
*** First throw call stack:
(
    0   CoreFoundation                      0x0000000111d45e65 __exceptionPreprocess + 165
    1   libobjc.A.dylib                     0x00000001117bedeb objc_exception_throw + 48
    2   CoreFoundation                      0x0000000111d45d9d +[NSException raise:format:] + 205
    3   Foundation                          0x000000011139a5ee -[NSMutableRLEArray replaceObjectsInRange:withObject:length:] + 146
    4   Foundation                          0x00000001113b7bfb -[NSConcreteMutableAttributedString setAttributes:range:] + 95
    5   ActiveLabel                         0x000000010f2c450f _TFC11ActiveLabel11ActiveLabelP33_42C3D6786121DF647E596543A9DC427916addLinkAttributefS0_FCSo25NSMutableAttributedStringT_ + 2911
    6   ActiveLabel                         0x000000010f2cac31 _TFFC11ActiveLabel11ActiveLabelP33_42C3D6786121DF647E596543A9DC427917updateTextStorageFS0_FT9parseTextSb_T_U_FT_T_ + 49

@polqf
Copy link
Contributor Author

polqf commented Feb 4, 2016

Hi @JaviSoto ,

Seems related with #14 . I've been using the 0.4.0 version in a personal app, without issues. But @byAmmerix complained about it few days ago. It is possibly related with the dispatch_async.

I'll try to look at it, but let's follow this up in #14. Would you mind to give there a little bit of info about the environment you are using (sdk, ios version..)?

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants