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

[SE-NNNN] Simplify internals of Codable-related APIs #63172

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

benrimmington
Copy link
Collaborator

@benrimmington benrimmington commented Jan 23, 2023

Proposal SE-NNNN:

  • Add two primary associated types?
    Replaces internal stdlib classes with constrained existentials.

  • Add a non-failable initializer?
    This could be used by the CodingUserInfoKey.actorSystemKey.

Resolves #49302

@benrimmington benrimmington added the swift evolution proposal needed Flag → feature: A feature that warrants a Swift evolution proposal label Jan 23, 2023
Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Nice!

If forced to, we could work around the api-digester false positives by adding these to the exceptions list.

@benrimmington
Copy link
Collaborator Author

@lorentey Thanks for the review. If only I hadn't mentioned those protocols during your pitch:

Are KeyedEncodingContainerProtocol and KeyedDecodingContainerProtocol commonly used?

No -- in fact, given that Encoder/Decoder sadly forces these into type-erased boxes, there is very little reason for anyone to write generic functions over them. Therefore, there is no reason for these to have a primary associated type. 😞

Would another proposal be worthwhile, even if the primary associated types will only be used internally?


If forced to, we could work around the api-digester false positives by adding these to the exceptions list.

#63184 (comment) had many more false positives. Can we trust api-digester to check for mangled name changes, because they shouldn't be affected by any and some syntactic sugar?

@lorentey
Copy link
Member

Would another proposal be worthwhile, even if the primary associated types will only be used internally?

I think this would need a short proposal, yes. Adding a primary associated type only to simplify internals should be a worthy enough goal.

Can we trust api-digester to check for mangled name changes, because they shouldn't be affected by any and some syntactic sugar?

The api-digester usually has low specificity, but very high sensitivity. So the rate of false positives is high, but false negatives are usually rare. (Or perhaps we just haven't realized yet that we've been shipping breaking changes. 😝)

@benrimmington
Copy link
Collaborator Author

@swift-ci Please benchmark

@benrimmington
Copy link
Collaborator Author

https://ci.swift.org/job/swift-PR-macos-perf/567/

Performance (x86_64): -O

Improvement OLD NEW DELTA RATIO
FlattenListLoop 1616.0 1046.5 -35.2% 1.54x (?)
Data.append.Sequence.809B.Count.RE.I 37.594 35.0 -6.9% 1.07x (?)
DictionaryKeysContainsNative 15.394 14.38 -6.6% 1.07x (?)

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
StringWithCString2 0.001 0.002 +50.0% 0.67x (?)
ArrayInClass 196.622 218.103 +10.9% 0.90x (?)
DistinctClassFieldAccesses 43.648 48.0 +10.0% 0.91x (?)
Array2D 4304.0 4715.429 +9.6% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
Set.isDisjoint.Seq.Box.Empty 90.4 83.88 -7.2% 1.08x (?)

Code size: -Osize

Performance (x86_64): -Onone

Improvement OLD NEW DELTA RATIO
CharacterLiteralsLarge 455.4 409.6 -10.1% 1.11x (?)
ObjectiveCBridgeStubNSDateRefAccess 3255.0 2981.0 -8.4% 1.09x (?)
ObjectiveCBridgeStubDateAccess 3118.0 2904.0 -6.9% 1.07x (?)

Code size: -swiftlibs

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 mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 32 GB

@benrimmington
Copy link
Collaborator Author

The internal base classes were made non-generic in #31278. Would removing them be a regression? The results didn't show any differences in either code size; or in the performance of CodableTest.swift benchmarks.

@benrimmington benrimmington changed the title [stdlib] Update Codable.swift [SE-NNNN] Simplify internals of Codable-related APIs Mar 1, 2023
@benrimmington
Copy link
Collaborator Author

@swift-ci Please smoke test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution proposal needed Flag → feature: A feature that warrants a Swift evolution proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SR-6753] CodingUserInfoKey's init?(rawValue:) shouldn't be failable
2 participants