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.

Sign up for GitHub

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

Jump to bottom

Manually specialize parts of CRC32 implementation to speed them up debug mode #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Manually specialize parts of CRC32 implementation to speed them up debug mode #19

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 75 additions & 1 deletion Sources/CRC/CRC32.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,90 @@ struct CRC32:Hashable, Sendable
return checksum
}

/// Returns a new checksum by hashing the provided message into the current checksum.
///
/// This manually specialized implementation is much faster in debug mode than the
/// generic implementation, but exactly the same in release mode.
@inlinable public
Copy link
Owner

Choose a reason for hiding this comment

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

should this be consuming, and should there also be a generic Sequence<UInt8> overload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already a generic overload above (which this manually specified method is kinda a copy of). I don't think consuming would make a difference to debug mode performance, but I do agree that it'd make sense for both the existing method and this method to have.

Copy link
Owner

Choose a reason for hiding this comment

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

this type is bitwise copyable, so consuming would not affect performance, it is more a matter of “usage hint”. it’s also a bit simpler to spell the body of the function, as you simply mutate self and return it

please do add a consuming func updated(with message:borrowing some Sequence<UInt8>), somebody (probably myself) will inevitably reach for it

also, since these are performance hooks, i would really prefer if the [UInt8] overloads were prefixed with an underscore. overloading sucks in Swift, and it would be nice if documentation authors (both in this package and downstream) could still refer to update(with:) without having to guess an FNV-1 hash :)

func updated(with message:borrowing [UInt8]) -> Self
{
var checksum:Self = self
checksum.update(with: message)
return checksum
}

/// Updates the checksum by hashing the provided message into the existing checksum.
@inlinable public mutating
func update(with message:borrowing some Sequence<UInt8>)
{
self.checksum = ~message.reduce(~self.checksum)
{
(state:UInt32, byte:UInt8) in
Self.table[Int.init(UInt8.init(truncatingIfNeeded: state) ^ byte)] ^ state >> 8
let indexByte:UInt8 = UInt8.init(truncatingIfNeeded: state) ^ byte
Copy link
Owner

Choose a reason for hiding this comment

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

since the performance-sensitive caller is going through the [UInt8] overload, do we actually need this?

let index:Int
#if DEBUG
// in debug mode these hacky integer conversions make this function
// around 35% faster
if MemoryLayout<Int>.stride == 8 {
let tuple:(UInt8, UInt8, UInt8, UInt8, UInt8, UInt8, UInt8, UInt8) =
(
indexByte, 0, 0, 0, 0, 0, 0, 0
)
index = unsafeBitCast(tuple, to: Int.self)
} else {
let tuple:(UInt8, UInt8, UInt8, UInt8) =
(
indexByte, 0, 0, 0
)
index = unsafeBitCast(tuple, to: Int.self)
}
#else
index = Int.init(indexByte)
#endif
return Self.table[index] ^ state >> 8
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

out of curiosity, can we get away with a [UInt8] typed overload that calls into the generic Sequence<UInt8> overload?

this ought to be enough to trigger specialization if i remember correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is specifically a debug mode optimisation, and I don't think the compiler really does any specialisation in debug mode. Optimising for debug mode and optimising for release mode seem to be completely different beasts. As soon as a generic function is called, even if it's in the standard library, there seems to be a bunch of overhead. Here of course the overhead isn't at the call-site of that function, but it accumulates on every step of reduce due to the generic iterator and stuff.

Copy link
Owner

Choose a reason for hiding this comment

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

okay

/// Updates the checksum by hashing the provided message into the existing checksum.
///
/// This manually specialized implementation is much faster in debug mode than the
/// generic implementation, but exactly the same in release mode.
@inlinable public mutating
func update(with message:borrowing [UInt8])
{
#if DEBUG
// in debug mode this manually specialized version of `reduce` is about 2.8x faster
self.checksum = ~self.checksum
var i:Int = 0
while i < message.count
{
let state:UInt32 = self.checksum
let byte:UInt8 = message[i]
let indexByte:UInt8 = UInt8.init(truncatingIfNeeded: state) ^ byte
let index:Int
// in debug mode these hacky integer conversions make this function
// around 35% faster
if MemoryLayout<Int>.stride == 8 {
let tuple:(UInt8, UInt8, UInt8, UInt8, UInt8, UInt8, UInt8, UInt8) =
(
indexByte, 0, 0, 0, 0, 0, 0, 0
)
index = unsafeBitCast(tuple, to: Int.self)
} else {
let tuple:(UInt8, UInt8, UInt8, UInt8) =
(
indexByte, 0, 0, 0
)
index = unsafeBitCast(tuple, to: Int.self)
}
self.checksum = Self.table[index] ^ state >> 8
i += 1
}
self.checksum = ~self.checksum
#else
self.update(with: message[...])
#endif
}
}
extension CRC32:ExpressibleByIntegerLiteral
{
Expand Down
Loading