[SE-0206][stdlib] Implement Hashable Enhancements proposal #16073
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lorentey
added
the
swift evolution approved
|
@swift-ci smoke test |
| @effects(releasenone) | ||
| public mutating func combine(bytes: UnsafeRawBufferPointer) { | ||
| _core.combine(bytes: bytes) | ||
| } | ||
|
|
||
| /// Finalize the hasher state and return the hash value. | ||
| /// Finalizing invalidates the hasher; additional bits cannot be combined | ||
| /// into it, and it cannot be finalized again. |
There was a problem hiding this comment.
Is this the final semantics for finalize()? I didn't totally understand the description in the acceptance notice.
There was a problem hiding this comment.
You're right, it isn't! I have to change it to be nonmutating but consuming. I don't think we need to change the name to finalized() because of the consuming semantics. @jckarter, do you agree?
There was a problem hiding this comment.
We didn't discuss changing the name. The discussion about the hasher being invalidated may still not be necessary anymore since the consuming means you can't use the finalized value anymore anyway.
|
OK, we now have a consuming finalize. I also made non-public declarations |
|
@swift-ci please test |
lorentey
commented
Apr 22, 2018
stdlib/public/core/Hasher.swift
Outdated
| /// Finalize the hasher state and return the hash value. Finalizing consumes | ||
| /// the hasher, forbidding further operations. | ||
| @effects(releasenone) | ||
| public __consuming func finalize() -> Int { |
There was a problem hiding this comment.
@jckarter, @natecook1000 A later commit changes it to read "Finalizing consumes the hasher, forbidding further operations", which is technically kind of true, but it's not the whole story. How about this?
public struct Hasher {
...
/// Finalize the hasher state and return the hash value.
///
/// Finalizing consumes the hasher: it is illegal to finalize a hasher you
/// don't own, or to perform operations on a finalized hasher. (These
/// may become compile-time errors in the future.)
@effects(releasenone)
public __consuming func finalize() -> Int {...}
}
...
public protocol Hashable {
...
/// Hash the essential components of this value into the hash function
/// represented by `hasher`, by feeding them into it using its `combine`
/// methods.
///
/// Essential components are precisely those that are compared in the type's
/// implementation of `Equatable`.
///
/// Note that `hash(into:)` doesn't own the hasher passed into it, so it must not
/// call `finalize()` on it. Doing so may become a compile-time error in the future.
func hash(into hasher: inout Hasher)
}
lorentey
changed the title
lorentey
force-pushed
the
make-hasher-public
branch
from
ea11724
to
c3b4902
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@swift-ci please test source compatibility |
|
cc @slavapestov 356e595a86c3528ccefdb2da965dc1153b2350ad and ad1255b24191b22f2a0b67df8b4ca8c1c1f21638 implement synthesis for the new Hashable. Adding |
lorentey
requested review from
slavapestov,
airspeedswift,
milseman,
moiseev and
lancep
lancep
approved these changes
Apr 24, 2018
| @@ -17,6 +17,9 @@ extension _CFObject { | |||
| public var hashValue: Int { | |||
| return Int(bitPattern: CFHash(self)) | |||
| } | |||
| public func hash(into hasher: inout Hasher) { | |||
| hasher.combine(self.hashValue) | |||
| } | |||
There was a problem hiding this comment.
Do we need this since the compiler auto-synthesizes it?
There was a problem hiding this comment.
CF types are magically auto-conformed to the _CFObject protocol, and synthesis doesn't work for them. Luckily this is an extremely special case that (as far as I know) only applies to this particular protocol. (It definitely doesn't affect any user code.) cc @jrose-apple
As noted in the proposal’s revision, this allows us to get rid of finalization checks, improves API robustness, and paves the way for making Hasher move-only in the future.
Newly internal declarations include Hasher._seed and the integer overloads of Hasher._combine(_:), as well as _SipHash13 and _SipHash24. Unify the interfaces of these SipHash testing structs with Hasher. Update SipHash test to cover Hasher, too. Add @usableFromInline to all newly internal stuff. In addition to its normal use, it also enables white box testing; compile tests that need to use these declarations with -disable-access-control.
This removes the default implementation of hash(into:), and replaces it with automatic synthesis built into the compiler. Hashable can now be implemented by defining either hashValue or hash(into:) -- the compiler supplies the missing half automatically, in all cases. To determine which hash(into:) implementation to generate, the synthesizer resolves hashValue -- if it finds a synthesized definition for it, then the generated hash(into:) body implements hashing from scratch, feeding components into the hasher. Otherwise, the body implements hash(into:) in terms of hashValue.
…or _hash(into:) Without this change, the Hashable synthesizer attempts to add hash(into:) methods directly on CF types, which (somewhat unsurprisingly) fails with assertion below. (This situation is unique to CF types, which are imported with an implicit _CFObject conformance; we usually have an extension context or a non-imported type decl in which to put the derived methods.) Assertion failed: (!decl->isForeign() && "Use getForeignMetadataLayout()"), function getClassMetadataLayout, file /Users/klorentey/Swift/swift/lib/IRGen/MetadataLayout.cpp, line 83. 0 swift 0x000000010ccd29c8 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 40 1 swift 0x000000010ccd30d6 SignalHandler(int) + 694 2 libsystem_platform.dylib 0x00007fff600e0d9a _sigtramp + 26 3 swift 0x000000010e04f1ed cmark_strbuf__initbuf + 122440 4 libsystem_c.dylib 0x00007fff5ffa6f79 abort + 127 5 libsystem_c.dylib 0x00007fff5ff6f090 basename_r + 0 6 swift 0x00000001096e8f95 swift::irgen::IRGenModule::getClassMetadataLayout(swift::ClassDecl*) + 53 7 swift 0x00000001095b9e25 swift::irgen::emitVirtualMethodValue(swift::irgen::IRGenFunction&, llvm::Value*, swift::SILDeclRef, swift::CanTypeWrapper<swift::SILFunctionType>) + 133 8 swift 0x00000001095ba252 swift::irgen::emitVirtualMethodValue(swift::irgen::IRGenFunction&, llvm::Value*, swift::SILType, swift::SILDeclRef, swift::CanTypeWrapper<swift::SILFunctionType>, bool) + 690 9 swift 0x00000001096bef49 swift::SILInstructionVisitor<(anonymous namespace)::IRGenSILFunction, void>::visit(swift::SILInstruction*) + 33497 10 swift 0x00000001096b3067 swift::irgen::IRGenModule::emitSILFunction(swift::SILFunction*) + 8055 11 swift 0x00000001095c8d8b swift::irgen::IRGenerator::emitLazyDefinitions() + 1051 12 swift 0x000000010968eb16 performIRGeneration(swift::IRGenOptions&, swift::ModuleDecl*, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::StringRef, swift::PrimarySpecificPaths const&, llvm::LLVMContext&, swift::SourceFile*, llvm::GlobalVariable**, unsigned int) + 1382 13 swift 0x000000010968f05e swift::performIRGeneration(swift::IRGenOptions&, swift::SourceFile&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::StringRef, swift::PrimarySpecificPaths const&, llvm::LLVMContext&, unsigned int, llvm::GlobalVariable**) + 94 14 swift 0x000000010952cfc0 performCompile(swift::CompilerInstance&, swift::CompilerInvocation&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*, swift::UnifiedStatsReporter*) + 15488 15 swift 0x0000000109528332 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 2962 16 swift 0x00000001094e4848 main + 2328 17 libdyld.dylib 0x00007fff5feecc41 start + 1 Stack dump: […] 1. While emitting IR SIL function "@$SSo10CGColorRefas8HashableSCsACP4hash4intoys6HasherVz_tFTW". for 'hash(into:)' in module 'CoreGraphics'
hash(into:) needs to be included in expectations; tests looking at synthesized Hashable implementation bodies need to be updated for resilient hashing.
…iated values
For enums with no associated values, it is better to move the hasher.combine call out of the switch statement, like this:
func hash(into hasher: inout Hasher) {
let discriminator: Int
switch self {
case a: discriminator = 0
case b: discriminator = 1
case c: discriminator = 2
}
hasher.combine(discriminator)
}
This enables the optimizer to replace the switch statement with a simple integer promotion, restoring earlier behavior.
lorentey
force-pushed
the
make-hasher-public
branch
from
c3b4902
to
110ee05
Compare
|
@swift-ci test source compatibility |
|
@swift-ci smoke test compiler performance |
|
@swift-ci benchmark |
|
@swift-ci test |
milseman
approved these changes
Apr 24, 2018
Build comment file:Optimized (O)Regression (16)
Improvement (30)
No Changes (376)
Unoptimized (Onone)Regression (17)
Improvement (29)
No Changes (376)
Hardware Overview |
|
Those are some nice boosts for Dictionary4; eliminating all those nested hashers helped a lot. The 5-6% improvements elsewhere might be due to the removal of the finalization precondition. (I'd have expected less than that, but I'll take it!) |
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the changes proposed in SE-0206, "Hashable Enhancements".
Hashertype andHashable's newhash(into:)requirement publicHashablewith new cases, as described below.This is fully source compatible with existing code. Code can choose to implement either Hashable requirement; the other requirement is then automatically synthesized by the compiler.
rdar://problem/35052153
Conforming to
Hashableby implementinghashValueCode written for previous versions of Swift conforms to
Hashableby implementinghashValue. This will keep working after this PR, with no source-level changes:To make this work, the compiler synthesizes the missing
hash(into:)requirement automatically:This partial synthesis is new with this PR. It works for all types that can implement
Hashable: enums, structs and classes, with no restrictions. (Deriving class members is now safe, thanks to @slavapestov's work in #16057.)Conforming to
Hashableby implementinghash(into:)I hope code written for 4.2+ will prefer to implement the
hash(into:)requirement instead:In this case the compiler will automatically provide the missing
hashValuerequirement, with the following implementation:Like the previous case, this kind of
hashValuesynthesis works for all types, with no restrictions.Conforming to
Hashableby automatic synthesisSince SE-0185, it has been possible to have the compiler supply the entire
Hashableconformance for structs and enums whose components are allHashable. For example:This will keep working after this PR, except the compiler now implements this conformance as follows:
Enums work, too:
(In the examples above, the compiler also synthesizes
Equatableconformance.)