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

stdlib: Print the error message in precondition() and preconditionFailure() even in release builds. #26215

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

stdlib: Print the error message in precondition() and preconditionFailure() even in release builds. #26215

wants to merge 1 commit into from

Conversation

eeckstein
Copy link
Member

@eeckstein eeckstein commented Jul 18, 2019 •
edited

This is important for frameworks, like Foundation, because frameworks are always shipped and used as release builds.
This change results in a little code size overhead, but it's quite small: Instead of a trap instruction it basically ends up in loading a few registers and calling the fatalError runtime function.

rdar://problem/71323149

@eeckstein
Copy link
Member Author

@swift-ci smoke test

@eeckstein
stdlib: Print the error message in precondition() and preconditionFai…
5b31183 
…lure() even in release builds.

This is important for frameworks, like Foundation, because frameworks are always shipped and used as release builds.
This change results in a little code size overhead, but it's quite small: Instead of a trap instruction it basically ends up in loading a few registers and calling the fatalError runtime function.

I also added an overload for precondition/Failure without a message (instead of a default parameter). This overload just traps, because without a message calling the fatalError function does not add any benefit.
xwu
xwu reviewed Jul 19, 2019
View reviewed changes
stdlib/public/core/Assert.swift
@@ -59,10 +59,12 @@ public func assert(
///
/// * In playgrounds and `-Onone` builds (the default for Xcode's Debug
/// configuration): If `condition` evaluates to `false`, stop program
/// execution in a debuggable state after printing `message`.
/// execution in a debuggable state after printing `message`
/// along with the file and line information..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// along with the file and line information..
/// along with the file and line information.

@airspeedswift
Copy link
Member

@swift-ci please benchmark

@airspeedswift
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Collaborator

Performance: -O

Regression OLD NEW DELTA RATIO
DataSubscriptSmall 13 54 +315.4% 0.24x
FlattenListLoop 2237 3007 +34.4% 0.74x (?)
DataCopyBytesSmall 61 80 +31.1% 0.76x
ObjectiveCBridgeStubFromNSStringRef 90 104 +15.6% 0.87x (?)
RandomShuffleLCG2 336 384 +14.3% 0.88x
Array2D 3472 3920 +12.9% 0.89x
MapReduceAnyCollection 196 217 +10.7% 0.90x (?)
MapReduce 196 216 +10.2% 0.91x
RemoveWhereSwapInts 31 34 +9.7% 0.91x (?)
DataCopyBytesMedium 252 274 +8.7% 0.92x (?)
RemoveWhereFilterInts 23 25 +8.7% 0.92x
 
Improvement OLD NEW DELTA RATIO
DataCountSmall 13 10 -23.1% 1.30x
Dictionary4 198 155 -21.7% 1.28x
Dictionary4OfObjects 227 196 -13.7% 1.16x
DataSubscriptMedium 37 32 -13.5% 1.16x (?)
DataCountMedium 17 15 -11.8% 1.13x
LuhnAlgoLazy 213 199 -6.6% 1.07x (?)

Code size: -O

Regression OLD NEW DELTA RATIO
ClassArrayGetter.o 5631 6300 +11.9% 0.89x
Prims.o 16829 18205 +8.2% 0.92x
PrimsSplit.o 16917 18293 +8.1% 0.92x
SortLettersInPlace.o 8919 9587 +7.5% 0.93x
DataBenchmarks.o 84534 90761 +7.4% 0.93x
DictionaryGroup.o 16632 17280 +3.9% 0.96x
MapReduce.o 32139 33275 +3.5% 0.97x
ObjectiveCBridging.o 59425 60093 +1.1% 0.99x

Performance: -Osize

Regression OLD NEW DELTA RATIO
DataSubscriptSmall 13 57 +338.4% 0.23x
FlattenListLoop 2213 2801 +26.6% 0.79x (?)
DataCopyBytesMedium 230 252 +9.6% 0.91x (?)
DataCopyBytesSmall 78 84 +7.7% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DataSubscriptMedium 39 32 -17.9% 1.22x (?)
IterateData 918 827 -9.9% 1.11x (?)
ObjectiveCBridgeStubFromNSStringRef 92 85 -7.6% 1.08x (?)

Code size: -Osize

Regression OLD NEW DELTA RATIO
ClassArrayGetter.o 5738 6391 +11.4% 0.90x
SortLettersInPlace.o 8505 9157 +7.7% 0.93x
DictionaryGroup.o 13910 14542 +4.5% 0.96x
MapReduce.o 25960 27048 +4.2% 0.96x
ObjectiveCBridging.o 53294 53930 +1.2% 0.99x

Performance: -Onone

Regression OLD NEW DELTA RATIO
DataSubscriptSmall 75 83 +10.7% 0.90x (?)

Code size: -swiftlibs

Regression OLD NEW DELTA RATIO
libswiftMapKit.dylib 36864 40960 +11.1% 0.90x
libswiftFoundation.dylib 1519616 1568768 +3.2% 0.97x
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: 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: 64 GB

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 5b31183

jrose-apple
jrose-apple reviewed Sep 12, 2019
View reviewed changes
stdlib/public/core/Assert.swift
} else if _isReleaseAssertConfiguration() {
Builtin.condfail_message(true._value,
StaticString("precondition failure").unsafeRawPointer)
StaticString("Precondition failed").unsafeRawPointer)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest not adding this additional overload. We don't really want people omitting messages from their preconditions to save on code size; that is an unnecessary level of flexibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't add it, but converted the default-argument version to a separate function. Removing the overload would be a source compatibility break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I'm saying you could just leave the default-argument version.

@svenmuennich
Copy link

Is there any chance for this PR to be picked up again? We just ran into the problem that a preconditionFailure() was triggered in a release build without printing the message first. In that particular case it would have been very helpful to see the message in the crash report.

@eeckstein
Copy link
Member Author

I think the better usability is worth the small code size increase.

@airspeedswift @kylemacomber are you fine with making this change?

@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@eeckstein eeckstein changed the base branch from master to main October 1, 2020 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xwu xwu xwu left review comments

@jrose-apple jrose-apple jrose-apple left review comments

@airspeedswift airspeedswift Awaiting requested review from airspeedswift

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@eeckstein @airspeedswift @swift-ci @svenmuennich @shahmishal @xwu @jrose-apple