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

Optimize NSString.NSRangeToByteRange(start:length:) #122

Merged
merged 6 commits into from
Dec 17, 2015

Conversation

norio-nomura
Copy link
Collaborator

Introduce ByteOffsetCache for calculating byte offset.
By applying this, linting KeychainAccess by SwiftLint-0.5.1 is reduced from:

swiftlint  21.87s user 0.24s system 97% cpu 22.611 total

to:

swiftlint  16.62s user 0.24s system 97% cpu 17.362 total

On Instruments, NSRangeToByteRange() is reduced from 6579ms:
screenshot 2015-12-14 11 38 48

to 315ms:
screenshot 2015-12-14 11 39 43

on that test.

@norio-nomura
Copy link
Collaborator Author

This is that I mentioned in #120.

@norio-nomura
Copy link
Collaborator Author

I think the performance issue that related to realm/SwiftLint#247 is almost resolved, and another performance issues that made SwiftLint slower than 0.4.0 are caused by rules.

@norio-nomura
Copy link
Collaborator Author

Today, I worried about that this PR would make leaks String instance.
But, it does not happen as I tested using Instruments. 😌

cache[location] = entry

// keep largest 5 entry
if cache.keys.count > 5 {
Copy link
Owner

Choose a reason for hiding this comment

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

is it really necessary to evict cache entries? An Int/Int keypair is minuscule in memory compared to the rest of the strings we're working with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I tested again.
I confirmed that evicting cache does not affect performance. 👍
I'll remove that.

@norio-nomura
Copy link
Collaborator Author

Thank you for reviews.
I updated.

@norio-nomura
Copy link
Collaborator Author

Today, I worried about that this PR would make leaks String instance.
But, it does not happen as I tested using Instruments. 😌

I also found that using NSString.byteOffsetCache cause memory leak if NSString is native NSString.
If NSString is casted from String, that does not cause memory leak.

Should we avoid using this approach?

@norio-nomura norio-nomura changed the title Optimize NSString.NSRangeToByteRange(start:length:) [WIP] Optimize NSString.NSRangeToByteRange(start:length:) Dec 15, 2015
@norio-nomura
Copy link
Collaborator Author

It seems I need to using two workarounds for resolving memory leaks.

  1. Check NSString is NSTaggedPointerString. It would stop using cache if the length is short enough.
  2. Create new instance of string for avoiding Circular reference.

@norio-nomura norio-nomura changed the title [WIP] Optimize NSString.NSRangeToByteRange(start:length:) Optimize NSString.NSRangeToByteRange(start:length:) Dec 15, 2015
@norio-nomura
Copy link
Collaborator Author

I added workarounds for memory leaks.

@norio-nomura
Copy link
Collaborator Author

Introduce `ByteOffsetCache` for calculating byte offset.
By applying this, linting KeychainAccess by SwiftLint-0.5.1 is reduced from:
```
swiftlint  21.87s user 0.24s system 97% cpu 22.611 total
```
to:
```
swiftlint  16.62s user 0.24s system 97% cpu 17.362 total
```

On Instruments, `NSRangeToByteRange()` is reduced from 6579ms to 315ms on that test.
Rename `entry` to `byteOffsetIndexPair`
Remove evicting `ByteOffsetIndexPair.cache`
There are two reasons for:
1. Avoid using associatedObject on NSTaggedPointerString (< 7 bytes) because that does not free associatedObject.
2. Using cache is overkill for short string.

There is no verified reason for the threshold "50", but I
If string is `Swift.String`, holding that into `ByteOffsetCache` does not cause Circular reference, because Casting `String` to `NSString` makes new `NSString` instance.
If string is native `NSString` instance, Circular reference happens on following:
```
self.utf8View = (string as String).utf8
```
Because the reference to `NSString` is holded by every casted `String`, their Views and Indices.
@norio-nomura
Copy link
Collaborator Author

rebased to master

jpsim added a commit that referenced this pull request Dec 17, 2015
Optimize `NSString.NSRangeToByteRange(start:length:)`
@jpsim jpsim merged commit e25df2d into jpsim:master Dec 17, 2015
@jpsim
Copy link
Owner

jpsim commented Dec 17, 2015

Awesome! Thanks for yet another solid contribution.

@norio-nomura
Copy link
Collaborator Author

😄

@norio-nomura norio-nomura deleted the byte-offset-cache branch December 18, 2015 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants