Skip to content

[stdlib] Implement the new _modify accessor in Dictionary.subscript #18974

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

Merged
merged 2 commits into from
Sep 10, 2018

Conversation

lorentey
Copy link
Member

This enables in-place mutations of the key-based subscript.
These get rid of one of two full hash table lookup operations (including hashing the key) relative to the getter+setter approach. They can also eliminate COW copies in code like this:

dictionary[foo]?.append(bar)

(Admittedly this isn’t a very useful example.)

This enables in-place mutations of the key-based subscript.
These get rid of one of two full hash table lookup operations (including hashing the key) relative to the getter+setter approach. They can also eliminate COW copies in code like this:

dictionary[foo]?.append(bar)

(Admittedly this isn’t a very useful example.)
@lorentey
Copy link
Member Author

@swift-ci smoke test

@lorentey
Copy link
Member Author

lorentey commented Aug 25, 2018

Tests, motivating examples are not yet done. Implementation details will improve with #18790.

@lorentey lorentey requested a review from airspeedswift August 25, 2018 01:13
@slavapestov
Copy link
Contributor

General question for @rjmccall - are we going to use this with arrays too, or are we going to keep addressors around as a language feature and synthesize a ‘modify’ out of them?

@rjmccall
Copy link
Contributor

Addressors can't completely disappear because we have to bootstrap unsafe pointers somehow. But yes, #18840 can synthesize a modify from a mutable addressor.

@airspeedswift
Copy link
Member

@swift-ci please smoke benchmark staging

@swift-ci
Copy link
Contributor

Build comment file:

Performance: -O

TEST OLD NEW DELTA SPEEDUP
Improvement
IterateData 1756 1566 -10.8% 1.12x

Performance: -Osize

TEST OLD NEW DELTA SPEEDUP
Improvement
IterateData 1904 1690 -11.2% 1.13x (?)
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

@airspeedswift
Copy link
Member

Huh. If we really were avoiding double-hashing, I'd have expected more signal from the benchmarks. Surely we must be doing lots of dictionary subscript assigns in them.

@lorentey
Copy link
Member Author

We have benchmarks setting values, but simple subscript assigns (dict[foo] = bar) already hash just once - they don’t benefit from the new accessor. To demonstrate an improvement, we need to find an algorithm that performs in-place mutations on Optionals.

// up inserting it. Otherwise this can only be a removal or an in-place
// mutation. (If we guess wrong, we might needlessly rehash; that's fine.)
let (_, rehashed) = _variant.ensureUniqueNative(
withCapacity: self.capacity + (found ? 0 : 1),
Copy link
Member Author

@lorentey lorentey Aug 25, 2018

Choose a reason for hiding this comment

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

Good thing benchmarks don’t exercise this yet — this should use self.count, not capacity. (As it stands, this causes a full 2x storage reallocation & rehash on every insertion.) 😳

@airspeedswift
Copy link
Member

airspeedswift commented Aug 27, 2018

I guess the trouble is that nearly all the useful in-place mutations happen via the subscript(_:default:) version. dictionary[foo]?.append(bar) isn't useful but dictionary[foo, default: []).append(bar) is. As is histogram[name, default: 0] += 1.

@jrose-apple
Copy link
Contributor

You should be able to use basically the same logic for subscript(_:default:), no? By the time you get to a modify accessor, you know you want to do the writeback.

@rjmccall
Copy link
Contributor

rjmccall commented Aug 28, 2018

subscript(_:default:) is already implemented with a mutable addressor. It could switch to a read instead of a getter, but that won't show any benefits until we optimize the SILGen of read a little.

It can switch to using an unsafe addressor, though.

@airspeedswift
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 653b68c

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 653b68c

@airspeedswift
Copy link
Member

Trying again now #19085 is merged.

@swift-ci please test

@airspeedswift
Copy link
Member

@swift-ci please smoke test

@airspeedswift
Copy link
Member

@swift-ci please smoke benchmark

@airspeedswift
Copy link
Member

Bout to merge this, just testing it one more time since it's been a couple of weeks

@lorentey
Copy link
Member Author

@swift-ci smoke test and merge

@swift-ci
Copy link
Contributor

Build comment file:

Performance: -O

TEST OLD NEW DELTA SPEEDUP
Regression
COWArrayGuaranteedParameterOverhead 9720 11718 +20.6% 0.83x
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

@swift-ci swift-ci merged commit 77d25d8 into swiftlang:master Sep 10, 2018
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.

6 participants