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

ByteBufferView: implement most Collection/Sequence customisation points #1385

Closed
6 tasks done
weissi opened this issue Feb 6, 2020 · 7 comments
Closed
6 tasks done
Labels

Comments

@weissi
Copy link
Member

weissi commented Feb 6, 2020

ByteBufferView is a Collection of UInt8 which "views" into a ByteBuffer. Right now, we basically only implement the subscript which vends individual bytes. That's good enough to implement Collection/Sequence (and a bunch of more specialised sub-protocols) but it's not very fast at all.

Especially methods like firstIndex(of:) or firstIndex(where:) will be very slow because it'll repeatedly call the subscript for every byte. It would be much faster to just forward the Collection/Sequence customisation points directly to the underlying bytes.

Generally, this will look something like

public func _customTHEFUNCTION(ARGUMENTS) {
        return try self._buffer.withVeryUnsafeBytes { ptr in
            let ptr = UnsafeRawBufferPointer(start: ptr.baseAddress!.advanced(by: self._range.lowerBound), count: self._range.count))
            return ptr.THEFUNCTION(ARGUMENTS)
        }
}

so essentially we just forward all operations into the UnsafeRawBufferPointer to the bytes this ByteBufferView contains.

The Collection customisation points we should implement are

We also don't need to implement all of those in one go, this can be done in multiple PRs.

Some notes:

  • we should back the gains up by benchmarks
  • this requires basically no SwiftNIO knowledge but intermediate Swift knowledge about how protocols/Collection/Sequence
@weissi weissi added good first issue Good for newcomers performance labels Feb 6, 2020
@trungducc
Copy link
Contributor

I'm trying to resolve this task but it makes confused when you said

all three variants of _failEarlyRangeCheck by just checking the self._range

I thought that we should check bounds parameter instead of self._range. Did I misunderstand anything?

@weissi
Copy link
Member Author

weissi commented Feb 26, 2020

@trungducc thank you, that's awesome. I think this is just a misunderstanding. I think we need to compare that the bounds parameter of _failEarlyRangeCheck lies within self._range's bounds (self._range.lowerBound & self._range.upperBound) in ByteBufferView`.

@trungducc
Copy link
Contributor

@weissi Oh yes. That makes sense. I didn't think about it. Thank you for pointing out!

@Lukasa
Copy link
Contributor

Lukasa commented Feb 7, 2022

Added _copyContents in #2039. I don't think we need to do _copyToContiguousArray as the default implementation leans on _copyContents, so improving the latter improves the former, so I'm checking that off too.

Lukasa pushed a commit that referenced this issue Feb 9, 2022
* Add ByteBufferView contains benchmark.

* Add ByteBufferView contains benchmark.

* Add ByteBufferView contains benchmark.
@glbrntt
Copy link
Contributor

glbrntt commented Feb 9, 2022

_customContainsEquatableElement was done in #2044

@anishagg17
Copy link
Contributor

anishagg17 commented Jul 23, 2022

@trungducc thank you, that's awesome. I think this is just a misunderstanding. I think we need to compare that the bounds parameter of _failEarlyRangeCheck lies within self._range's bounds (self._range.lowerBound & self._range.upperBound) in ByteBufferView`.

Hi @weissi, I checked for CircularBuffer #2161 _failEarlyRangeCheck is a no-op as we were validating by use of Index.isValidIndex(). But in the case of ByteBufferView, I think we can implement _failEarlyRangeCheck as it can be utilized in the future. My only concern is whether it should have private access or public access

Edit: I've opened PR #2226 for this issue, at the same time I noticed that swift core has defined all 3 variants of _failEarlyRangeCheck(https://github.com/apple/swift/blob/main/stdlib/public/core/Collection.swift#L711-L734) in the default implementation of Collection (An indirect parent struct for ByteBufferView, in the inheritance hierarchy). Adding our own implementation of _failEarlyRangeCheck doesn't seem advantageous unless we have some plans of owning & customizing(in future) the functioning of _failEarlyRangeCheck

@Lukasa
Copy link
Contributor

Lukasa commented Apr 24, 2023

All subtasks here are completed.

@Lukasa Lukasa closed this as completed Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants