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

DIP1006: Providing more selective control over contracts #54

Merged
merged 1 commit into from
Apr 12, 2017
Merged

DIP1006: Providing more selective control over contracts #54

merged 1 commit into from
Apr 12, 2017

Conversation

mathias-lang-sociomantic
Copy link
Contributor

The aim of this DIP is to provide a common approach to handle deployment of applications in non-release mode.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Nice work! This is a solid and well written proposal.

DIPs/DIP1006.md Outdated
The code being very simple results in the class hierarchy to be traversed completely twice (no caching is done)
for every call, even for classes which do not define any invariant.

Experience showed that simply disabling this expensive lookup method can easily lead to 20% more throughput for OOP-intensive codebase.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest expanding here:

  1. with specific description of how it has affected Sociomantic project (within non-disclosure limits)
  2. Stressing that this difference came from invaraints alone, with no other optimizations changing

Choose a reason for hiding this comment

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

I would be interesting to also show some numbers for a public project, so it can be checked and peer-reviewed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestion for a good candidate ?

DIPs/DIP1006.md Outdated
Since this proposal was heavily motivated by the cost of invariant, an obvious alternative is to reduce said cost.
The compiler might be able to, for example, generate a virtual function which aggregates all of it's base class invariants,
resulting in a single indirection (virtual call) when no invariant is used.
While it's a direction that should be pursued, it is a different matter which should be pursued as a separate enhancement.
Copy link
Contributor

Choose a reason for hiding this comment

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

That is not a good motivation for adding new language feature (adding intermediate fix with assumption that the better fix may come later). Better to focus on the fact stated in next sentence and expand on reasons why handling invariants is likely to remain much slower by design (they have to be polymorphic which requires at least some runtime virtual table interaction)

DIPs/DIP1006.md Outdated
`assert` and `in` / `out` contracts follow a pay-for-what-you-use approach: an `assert` which is never hit is free,
and a `in` or `out` contract on a function that is never called is also free of runtime overhead.

`invariant` stand out in that regard: they can be used on a pay-for-what-you-use basis, by using `assert(object)` (or `assert(this)`),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be more readable to give a short introduction on how different kinds of contracts work (especially in relation to polymorphism), deriving performance consequences from that point.

@mihails-strasuns
Copy link
Contributor

Added few comments. Will also submit editorial suggestion PR soon-ish.

DIPs/DIP1006.md Outdated
The compiler might be able to, for example, generate a virtual function which aggregates all of it's base class invariants,
resulting in a single indirection (virtual call) when no invariant is used.
While it's a direction that should be pursued, it is a different matter which should be pursued as a separate enhancement.
The reason one might want to disable invariant is not because they are expensive, but because they are not a pay-for-what-you-use feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it also worth mentioning that some compilers already offer these options (like ldc), so it would make compilers more interoperable if a standard set of flags are defined.

#### Considered alternatives

The 4 values available to the user are voluntarily simple and hierarchical. It makes little sense to allow any contracts without enabling asserts.
It would be feasible to allow `invariant` without `in` and `out` contracts, or only `out` contracts, only `in` contracts, or some other combination.
Copy link

@PoignardAzur PoignardAzur Jan 24, 2017

Choose a reason for hiding this comment

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

Developers compiling a library might want to disable out contracts, because they are certain that their functions respect the stated post-conditions, while letting the in contracts enabled, because they don't trust the library users to respect the stated pre-conditions.

So having a "only in contracts + asserts" option does have an advantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I didn't think about this use case.
Since this still falls under the provided explanation for this proposal's simplicity (start with a simpler feature, expand as needed), I'll just slightly rephrase.

@wilzbach
Copy link
Member

What happened to this DIP? Ping @Dicebot

@mihails-strasuns
Copy link
Contributor

AFAICS whole DIP system got completely abandoned after I have left.

@mathias-lang-sociomantic
Copy link
Contributor Author

I can adjust the DIP according to the comment, but if there's not reviewer there would be no point in doing so.

@PetarKirov
Copy link
Member

This DIP is already in good enough state to merged. We can always improve the wording later.
@MartinNowak @wilzbach, can we get this merged so we can progress further?

@mathias-lang-sociomantic
Copy link
Contributor Author

I'd rather fix it beforehand. But the question is not about the state of this P.R., but the state of the DIP process as a whole. If no one's taking ownership of it, it's pointless to merge.

@wilzbach
Copy link
Member

@ZombineDev I have no merge rights for this repo, but as @mathias-lang-sociomantic stated we should fix the process first. This discussion probably belongs to the mailing list. However, here are my 2 cents to start this discussion. The options I see:
A) we find (a) new maintainer(s)
B) we adjust the process so that it works with minimal maintenance (e.g. by doing the review on the pending PR and only merging it if it has officially been approved or rejected)

@leandro-lucarella-sociomantic
Copy link
Contributor

AFAICS whole DIP system got completely abandoned after I have left.

Mike Parker is the new DIP czar

@mdparker
Copy link
Member

OK, the formal review of DIP 1003 is delayed for a bit, so my first act as the new DIP guy is going to be getting the preliminary review kicked off on this one.

@mathias-lang-sociomantic Ready to roll?

The aim of this DIP is to provide a common approach to handle deployment of applications in non-release mode.
@mathias-lang-sociomantic
Copy link
Contributor Author

Updated:

diff --git a/DIPs/DIP1006.md b/DIPs/DIP1006.md
index 3c8197c..dd60f30 100644
--- a/DIPs/DIP1006.md
+++ b/DIPs/DIP1006.md
@@ -46,7 +46,7 @@ and `invariant` is meant for expensive sanity checking. `in` and `out` contracts
 
 #### Cost of contracts
 
-`assert` and `in` / `out` contracts follow a pay-for-what-you-use approach: an `assert` which is never hit is free,
+`assert` and `in` / `out` contracts follow a pay-for-what-you-use approach: an `assert` which is never executed is free,
 and a `in` or `out` contract on a function that is never called is also free of runtime overhead.
 
 `invariant` stand out in that regard: they can be used on a pay-for-what-you-use basis, by using `assert(object)` (or `assert(this)`),
@@ -55,7 +55,9 @@ The call itself is not direct: the compiler inserts a call to `_d_invariant`, [w
 The code being very simple results in the class hierarchy to be traversed completely twice (no caching is done)
 for every call, even for classes which do not define any invariant.
 
-Experience showed that simply disabling this expensive lookup method can easily lead to 20% more throughput for OOP-intensive codebase.
+Profiling some real-time applications within Sociomantic showed that `_d_invariant` was the most expensive call in the application.
+Affected applications were using little or no invariant, but since Sociomantic code is mostly writen in an OOP style,
+simply disabling invariants (with no other optimization) led to 20% more throughput.
 
 #### Issues with the current approach
 
@@ -71,14 +73,13 @@ Any OOP-intensive code will want to get rid of `invariant` as a first step, whil
 
 The 4 values available to the user are voluntarily simple and hierarchical. It makes little sense to allow any contracts without enabling asserts.
 It would be feasible to allow `invariant` without `in` and `out` contracts, or only `out` contracts, only `in` contracts, or some other combination.
-Since providing all combinations would increase complexity, but doesn't provide an obvious advantage, it was left out of this proposal (but can be subject to another one).
+Since providing all combinations would increase complexity, but doesn't yet provide an obvious advantage, it was left out of this proposal (but can be subject to another one).
 
 Since this proposal was heavily motivated by the cost of invariant, an obvious alternative is to reduce said cost.
-The compiler might be able to, for example, generate a virtual function which aggregates all of it's base class invariants,
-resulting in a single indirection (virtual call) when no invariant is used.
-While it's a direction that should be pursued, it is a different matter which should be pursued as a separate enhancement.
-The reason one might want to disable invariant is not because they are expensive, but because they are not a pay-for-what-you-use feature.
+However, the cost of having `invariant` enabled will never be null for builds that do not use invariants at all, which is the real motivation for this feature.
 
+Finally, this functionality is already implemented in LDC via `-enable-invariants={0,1}`.
+Standardizing it would simplify user's life and allow tooling that deals with multiple compilers (e.g. `dub`, IDEs...) to provide this option.
 
 ### Breaking changes / deprecation process

Only comment not addressed is @Dicebot 's here, as I wasn't sure what was wanted.

@mdparker
Copy link
Member

Ping @Dicebot

@mihails-strasuns-sociomantic
Copy link
Contributor

It was just an idea that it would help to explain how polymorphism makes in/invariant (especially latter) impact on performance much more drastical than naively expected. Totally optional.

(The @Dicebot account is mostly abandoned)

@mdparker
Copy link
Member

OK, @mathias-lang-sociomantic , when you give me the thumbs up that you're ready, I'll kick this off in the forums.

@mathias-lang-sociomantic
Copy link
Contributor Author

@mdparker 👍 (ready)

@mdparker
Copy link
Member

Thanks!

@mdparker mdparker merged commit 61cef4e into dlang:master Apr 12, 2017
@ibuclaw
Copy link
Member

ibuclaw commented May 1, 2017

simply disabling invariants (with no other optimization) led to 20% more throughput.

That reminds me of an optimization I proposed last year on the NG.

D-Programming-GDC/gdc#132

Never got round to benchmarking it properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants