[stdlib] Set, Dictionary: Enable per-instance hash seeds #19533

lorentey · 2018-09-25T14:45:23Z

Set and Dictionary operations that copy elements from two hash tables of the same size are still quadratic with certain load factors.

This makes simple-looking loops like this extremely slow: (adapted from stdlib’s current implementation for union)

let count = 625_000
var a = Set(0 ..< count)
let b = Set(count ..< 2 * count)
for element in b {
  a.insert(element)
}

This is because a and b above share the same hash seed, both being sets of the same size. This causes the loop to fill the hash table of a extremely unevenly; the initial part of a’s hash table gets a load factor well over 100% — essentially converting it into an unsorted array until the overall alpha increases enough to trigger a storage resize and full rehash.

This performance pitfall can be eliminated by choosing a new hash seed for every new Set/Dictionary, so we should do that. (Except for copy-on-write copies, of course.) This makes the above code 11 times faster:

$ time swift -O /tmp/set.swift  # before
        7.89 real         7.72 user         0.11 sys
$ time swift -O /tmp/set.swift # after
        0.67 real         0.58 user         0.06 sys

See below: log-log chart of amortized per-element performance of Set.union for two non-overlapping sets of the same size, before & after this change.

rdar://problem/44760778

lorentey · 2018-09-25T14:46:44Z

@swift-ci please test

lorentey · 2018-09-25T14:47:42Z

We may or may not have benchmarks that trigger this issue. SetUnion etc. use an element count of 400, which is too low.

@swift-ci please smoke benchmark

swift-ci · 2018-09-25T15:34:28Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Improvement
IterateData	1727	1563	-9.5%	1.10x (?)
OpenClose	74	68	-8.1%	1.09x

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryLiteral.o	1385	1420	+2.5%	0.98x
Improvement
DictTest2.o	20792	20472	-1.5%	1.02x
DictTest3.o	29816	29496	-1.1%	1.01x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Improvement
IterateData	1833	1637	-10.7%	1.12x

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryLiteral.o	1491	1541	+3.4%	0.97x
Improvement
ObjectiveCBridging.o	45670	44430	-2.7%	1.03x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryKeysContainsNative	49	63	+28.6%	0.78x

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

stephentyrone · 2018-09-25T17:14:17Z

stdlib/public/core/HashTable.swift

+      // When we're using deterministic hashing, the scale value as the seed is
+      // still allowed, and it covers most cases. (Unfortunately some operations
+      // that merge two similar-sized hash tables will still be quadratic.)
+      return scale


If we wanted to get rid of quadratic behavior in the deterministic hashing case, we would use a (per-thread?) atomic counter here, I guess, though that's not quite strong enough for some cases? Do we care?

That's a really clever idea. Not sure it's worth the trouble though -- deterministic hashing is intended as a debugging/testing tool, and it's supposed to never be enabled in production.

What if we used an atomic counter for all cases?

stephentyrone

LGTM. This is a nice improvement for a hard-to-notice perf problem.

lorentey · 2018-09-25T17:33:35Z

Ugh, that's funny -- it looks like this actually breaks the package manager. 😮

lorentey · 2018-09-27T19:49:41Z

No, that was my fault -- I broke mapValues by accident.

This was one of those rare bugs that don't happen with deterministic hashing, which is why it wasn't caught by the test suite.

lorentey · 2018-09-27T19:57:24Z

This was one of those rare bugs that don't happen with deterministic hashing, which is why it wasn't caught by the test suite.

However, using an atomic counter as the seed would've definitely caught the issue. One more reason to add one?

lorentey · 2018-09-27T20:14:50Z

@swift-ci please test

lorentey · 2018-09-27T20:18:52Z

@swift-ci please test source compatibility

swift-ci · 2018-09-27T21:42:47Z

Build failed
Swift Test Linux Platform
Git Sha - 09c24f5a33fe4da7c8443a4a3359b2c8cf237410

lorentey · 2018-09-28T14:07:00Z

@swift-ci please test

swift-ci · 2018-09-28T15:15:24Z

Build failed
Swift Test Linux Platform
Git Sha - 603ded3a4447471d9b2a04cee0cee8c1fb5bb1c1

lorentey · 2018-09-28T20:09:17Z

0:15:16 /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swiftpm/Tests/PackageLoadingTests/PD4_2LoadingTests.swift:392: error: PackageDescription4_2LoadingTests.testCaching : XCTAssertEqual failed: ("[AbsolutePath:"/tmp/spm-tests-testCaching.H9akuF/pkg/Package.swift"]") is not equal to ("[]") -
10:15:16 /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swiftpm/Tests/PackageLoadingTests/PD4_2LoadingTests.swift:392: error: PackageDescription4_2LoadingTests.testCaching : XCTAssertEqual failed: ("[AbsolutePath:"/tmp/spm-tests-testCaching.H9akuF/pkg/Package.swift"]") is not equal to ("[]") -

Hm, there could be a correctness issue with one of the high-level operations. Weird that it only happens on Linux, though.

lorentey · 2018-09-28T20:09:30Z

@swift-ci please test

swift-ci · 2018-09-28T21:32:55Z

Build failed
Swift Test Linux Platform
Git Sha - 603ded3a4447471d9b2a04cee0cee8c1fb5bb1c1

The performance of operations merging two similar-sized sets or dictionaries is currently quadratic. Enabling per-instance seeds makes most of these linear, which is what most people expect. This affects Set.union, Set.symmetricDifference and similar operations, including the for-loop based forms that people write manually. The only remaining quadratic cases are when two sets/dictionaries are merged that are mutated copy-on-write copies of the same original instance. (We can’t change the hash seed without a full rehash,

lorentey · 2018-10-02T14:05:24Z

@swift-ci please test

swift-ci · 2018-10-02T15:23:01Z

Build failed
Swift Test Linux Platform
Git Sha - 08e0ad6

lorentey · 2018-10-04T16:02:01Z

I suspect the Linux package manifest caching test failures might be caused by an unrelated issue with the filesystem timestamp resolution -- except they seem to be reliably triggered by this change somehow. It's all somewhat bizarre; perhaps there's a Foundation issue?

@aciidb0mb3r, can you think of a reason why manifest caching would fail when the order of elements in a Set/Dictionary changes between instances?

aciidgh · 2018-10-04T16:30:55Z

hmm, nope. It may be something very subtle somewhere. Do you want me to investigate?

lorentey · 2018-10-04T16:49:34Z

Thanks! I have a couple of ideas to narrow it down a bit — I’ll let you know if I hit a wall. 👍

lorentey · 2018-10-05T19:51:31Z

The failure is because JSONEncoder is not deterministic on Linux. (Under the hood, it puts encoded data in dictionaries, which isn't a great choice. It should be encoding things on the fly, without constructing intermediate data structures and randomly reordering stuff.)

The Package Manager uses JSONEncoder to make keys for llbuild rules, and with per-instance seeding, these keys become unstable.

Fixing this is easy in SPM -- just encode keys with a different encoder. But this shouldn't be necessary -- JSONEncoder should be stable and fast.

aciidgh · 2018-10-05T20:25:44Z

@lorentey JSONEncoder does have a .sortedKeys option for output formatting. Do you think that'll fix the issue or is that broken on linux?

lorentey · 2018-10-06T01:50:31Z

@aciidb0mb3r Oh, nice, that should fix it! I did not know about that option. 👌

I submitted swiftlang/swift-package-manager#1802 to verify that enabling it resolves the test failure.

lorentey · 2018-10-06T01:51:13Z

Please test with the following PR:
swiftlang/swift-package-manager#1802
@swift-ci please test linux platform

lorentey · 2018-10-06T11:28:25Z

@swift-ci test and merge

lorentey requested review from airspeedswift and stephentyrone September 25, 2018 14:54

This comment has been minimized.

Sign in to view

stephentyrone reviewed Sep 25, 2018

View reviewed changes

stephentyrone approved these changes Sep 25, 2018

View reviewed changes

This was referenced Sep 27, 2018

[WIP] Dictionary/Set cumulative test PR #19586

Closed

[stdlib] Set, Dictionary: Prepare for per-instance hash seeds #19589

Merged

lorentey force-pushed the per-instance-seeding branch from cc46dea to 2381183 Compare September 27, 2018 19:52

lorentey force-pushed the per-instance-seeding branch 2 times, most recently from eaba464 to 09c24f5 Compare September 27, 2018 20:14

This comment has been minimized.

Sign in to view

lorentey force-pushed the per-instance-seeding branch from 09c24f5 to 603ded3 Compare September 28, 2018 14:06

lorentey force-pushed the per-instance-seeding branch from 603ded3 to 08e0ad6 Compare October 2, 2018 13:53

This comment has been minimized.

Sign in to view

swift-ci merged commit f9a2d90 into swiftlang:master Oct 6, 2018

lorentey deleted the per-instance-seeding branch October 9, 2018 16:47

[stdlib] Set, Dictionary: Enable per-instance hash seeds #19533

[stdlib] Set, Dictionary: Enable per-instance hash seeds #19533

Uh oh!

Conversation

lorentey commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorentey commented Sep 25, 2018

Uh oh!

lorentey commented Sep 25, 2018

Uh oh!

swift-ci commented Sep 25, 2018

Build comment file:

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Uh oh!

This comment has been minimized.

This comment has been minimized.

stephentyrone Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorentey Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

stephentyrone left a comment

Choose a reason for hiding this comment

Uh oh!

lorentey commented Sep 25, 2018

Uh oh!

lorentey commented Sep 27, 2018

Uh oh!

lorentey commented Sep 27, 2018

Uh oh!

lorentey commented Sep 27, 2018

Uh oh!

This comment has been minimized.

lorentey commented Sep 27, 2018

Uh oh!

This comment has been minimized.

swift-ci commented Sep 27, 2018

Uh oh!

lorentey commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

lorentey commented Sep 28, 2018

Uh oh!

lorentey commented Sep 28, 2018

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

lorentey commented Oct 2, 2018

Uh oh!

This comment has been minimized.

swift-ci commented Oct 2, 2018

Uh oh!

lorentey commented Oct 4, 2018

Uh oh!

aciidgh commented Oct 4, 2018

Uh oh!

lorentey commented Oct 4, 2018

Uh oh!

lorentey commented Oct 5, 2018

Uh oh!

aciidgh commented Oct 5, 2018

Uh oh!

lorentey commented Oct 6, 2018

Uh oh!

lorentey commented Oct 6, 2018

Uh oh!

lorentey commented Oct 6, 2018

Uh oh!

Uh oh!

lorentey commented Sep 25, 2018 •

edited

Loading

stephentyrone Sep 25, 2018 •

edited

Loading