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

replace @setRuntimeSafety with @optimizeFor #978

Open
andrewrk opened this issue May 3, 2018 · 10 comments
Open

replace @setRuntimeSafety with @optimizeFor #978

andrewrk opened this issue May 3, 2018 · 10 comments
Labels
accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented May 3, 2018

Communicate intent precisely.

Right now you can use @setRuntimeSafety(false) in Debug and ReleaseSafe modes to allow the optimizer to speed up particular blocks of code. This would make sense to do in a well tested high performance crypto algorithm for example. Or in general the bottlenecks of your library or program.

So the pattern here is that the build mode specifies the default, and you can override it at a per-block (or function) level. Here's a table of the parameters and the defaults that the build modes set:

Parameter Debug ReleaseFast ReleaseSafe ReleaseSmall
LLVM Optimizations - improve speed, harm debugging & compile time -O3 -O3 -Os
Runtime Safety Checks - harm speed, harm size, improve debugging On On

With @setRuntimeSafety we can make a block of code adhere to the principles of Debug even in a ReleaseFast build, and vice versa. ...to some extent - it does not affect optimizations.

But it could! LLVM supports choosing -O3, -Os, or -O0 at least at the function level.

You can probably guess what I'm leading up to here. Really instead of specifically asking to turn off safety, what we really want to communicate to the compiler is what we want to optimize for.

@optimizeFor(comptime mode: BuildMode)

Where BuildMode is already defined as @import("builtin").BuildMode:

pub const Mode = enum {
    Debug,
    ReleaseSafe,
    ReleaseFast,
    ReleaseSmall,
};

And then the other issue we need to solve here is that the same intent should be communicable for types. Zig reserves the right to order fields and insert padding and such for structs, and therefore we should also allow the programmer to tell Zig what to optimize for.

For example Debug might randomly order fields to make sure one does not accidentally depend on field order. ReleaseFast might arrange fields in an order most likely to have better cache performance. ReleaseSmall might sort fields by descending alignment to minimize padding.

One little tidbit: ReleaseSmall is about generated binary size, not runtime memory size. So there is a slight disconnect here. But "small" is definitely something you would potentially want to optimize for in a struct, union, or enum. We can think about how to name the release modes and the optimization targets more coherently.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label May 3, 2018
@andrewrk andrewrk added this to the 0.4.0 milestone May 3, 2018
@alexnask
Copy link
Contributor

alexnask commented May 3, 2018

@andrewrk Just a small correction, I believe the settings I used for ReleaseSmall are equivalent to -Oz.
(The PMBuilder->SizeLevel is 1 for -Os, 2 for -Oz)

@hryx
Copy link
Contributor

hryx commented Jul 8, 2019

Can you expand on the use case for leaving in @optimizeFor(.Debug) when shipping to production? If I understand, disabling optimizations is most useful so that you can see the code as it was written (no inlining, loop unrolling, vectorization, etc.) in a debugger. Do those matter when shipping?

You mention this cool idea:

For example Debug might randomly order fields to make sure one does not accidentally depend on field order

which is how precisely Go maps behave. But is that perhaps more of a safety feature than an optimization (or debugging) feature?

@shawnl
Copy link
Contributor

shawnl commented Oct 14, 2019

@hryx it would be really useful for debugging to be able to turn of optimizations for a specific part of code. You can do this in C.

Also it doesn't seem to be addressed here, but a struct has to be the same for the whole program, so where is its mode set?

@LemonBoy
Copy link
Contributor

But it could! LLVM supports choosing -O3, -Os, or -O0 at least at the function level.

Not quite, you can ask not to optimize some functions (optnone) or to minimize its size (optsize) but you can't really have, say, your whole project compiled with -O0 and only one function compiled with -O3.

@hvenev
Copy link

hvenev commented Jan 16, 2021

I think runtime safety should be independent of the mode of optimization. @setRuntimeSafety can be used as an indication of whether the code is known to be correct and the overhead of runtime checks is not worth it.

For example, in a release build it may make sense to compile hot spots with -O3 and no runtime checks, and use -Os with runtime checks enabled for (most of) the rest of the code.

@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 20, 2021
@nektro
Copy link
Contributor

nektro commented Jan 25, 2022

love it! would recommend @optimizeAs rather than @optimizeFor

@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Apr 16, 2022
@hdante
Copy link
Contributor

hdante commented Jul 8, 2022

Hello, my suggestion is replacing the generic optimization attribute by something like @codegen, which allows more fine grained optimization. gcc provides that with #pragma GCC optimize. For example, I'd like to replace large jump tables created by switch() with smaller code, but keep all other optimizations for speed:

func parse_cmdline(cmdline: []u8) codegen("-fno-jump-tables") !Configuration;

@matu3ba
Copy link
Contributor

matu3ba commented Jan 4, 2023

This proposal does not clarify how the standard syntax for tests would then look like. It would be very ugly/annoying, if it is more than 1 line. As of now it is @setRuntimeSafety(builtin.is_test);.

@sergey6661313
Copy link

the automatic security is great, I like most of the warnings.
but i would like to be able to change the hidden tag for union in safe mode.

What I mean: sometimes you want to have one field for two different states.
union(Tag) {init: void, a: A, b: A, c: C};
now the semantics of the language implies that the only "correct way" to assign a hidden tag to a union is something like this:

current_tag_data = &my_union.a;
my_union = .{.b = current_tag_data.*};

But this semantics shows explicit memory copying. But we weren't going to copy the memory, were we?
Moreover, memory copying is missing in release mode. (I looked at the output in godbolt - there is an explicit memcpy... in debug mode, but not in release mode anymore). I really don't like it when code doesn't do what it looks like. If copying is written, there must be copying.

however, it is obvious that I do not need copying ... If the structure implies one field, it is obvious that it should be read, i.e.:
use the usual hand made union and tag separately from it.
tag {init, a, b, c};
union {init: void, a: A, c: C};
but naturally I got the error "access of union field, where tag is init" at every step, because I could not change the hidden tag inside the union which was NOT even tagged...

Using release doesn't suit me because the debug mod with all the other built-in checks is exactly what I need at the development stage. Rather, even I would like to have some checks in release mode too.

  1. I lose that very security,
  2. long compilation time.

I read about extern and packed unions, they do not suit me because:

  1. I lose that very security,
  2. I lose automatic padding inside the structure,
  3. I lose the ability to use ordinary structures inside packed and extern structures.

Now I have placed a lot of @setRuntimeSafety(false) almost everywhere in the code before each reading of this structure. Instead of all this, I would like to be able to simply change the hidden tag of the union like this:
@setHiddenTag(&my_union, .b);
This will make my code safer and more explicit.
Obviously, in the release mod, it maybe simply be ignored. Especially since I started using regular union rather than tagget union and still get errors related to tag...
especially since there is no way to track exactly where in the code this variable changed if it was allocated on the heap and stored in a dynamic array...

@mlugg
Copy link
Member

mlugg commented Jan 30, 2023

now the semantics of the language implies that the only "correct way" to assign a hidden tag to a union is something like this:

This code is overcomplicated. You can simply write my_union = .{ .b = my_union.a };.

I really don't like it when code doesn't do what it looks like. If copying is written, there must be copying.

Optimizations naturally suppress copies of data in basically every language. In C, for instance, *x = 1; *x += 1 will more than likely not actually dereference x twice. In a more similar vein to this, the C code my_struct = { .x = my_struct.x, .y = 3 }; would likely elide the copy of x and only modify y. This is a natural part of optimization, and relying on it in basic cases like this is fine.

Moreover, the point of this rule is that there could be copying. Zig reserves the right to lay out auto-layout (i.e. not packed or exern) unions however it likes, and this includes putting identically-typed payloads for distinct tags in different places. As an example of how something like this could be useful, consider union { a: u7, b: u7 } - here, the single-bit tag can go in the unused high-order bit to make this value take 1 byte, so while the bits are in the same place, the tag actually overlaps the data. There are probably (somewhat contrived) examples where the tag becomes even more interlaced with the data, making "set the tag" a non-trivial operation which may genuinely involve copying several bytes of the payload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
Status: To do
Development

No branches or pull requests