Make assumeSafeAppend nothrow

[monarchdodra]

This makes assumeSafeAppend nothrow. AFAIK, it can't throw, so...

[MartinNowak]

I'd like to see the implementation of _d_arrayshrinkfit in rt.lifetime being marked as nothrow too then.

[monarchdodra]

Hum... I can was able to "cleanly" tag those two __blk functions. After that, I would have need to also mark some other extern C functions:

    extern (C) void*   gc_addrOf( in void* p );
    extern (C) size_t  gc_sizeOf( in void* p );
    extern (C) BlkInfo gc_query( in void* p );

But I wasn't able to find their source, so I'm no sure if I can tag them.

Even if I could, the keyword synchronized is creating this problem for me:

src/rt/lifetime.d(246): Error: '_d_monitorenter' is not nothrow
src/rt/lifetime.d(246): Error: '_d_monitorexit' is not nothrow

And I don't know how to fix that.

So... yeah... I wasn't actually able to mark them as nothrow...

[MartinNowak]

Those are in gc.proxy

src/rt/lifetime.d(246): Error: '_d_monitorenter' is not nothrow
src/rt/lifetime.d(246): Error: '_d_monitorexit' is not nothrow

These functions are defined in object. But we can't make Object.Monitor's lock and unlock nothrow.

[MartinNowak]

Maybe we should close this pull?

[monarchdodra]

Maybe we should close this pull?

The problem I think we have is that capacity and reserve are both marked nothrow. Yet they are just as capable of throwing exceptions if a mutex lock fails for a shared type...
...as for reserve itself, it can throw in for non-shared code if the postblit throws. If anything, assumeSafeAppend will throw less exceptions than reserve.

As long as capacity/reserve are both northow, I have a bit of problems with not having a nothrow assumeSafeAppend :(

[monarchdodra]

Hey, can you make sense of this?

extern(C) void _d_arrayshrinkfit(const TypeInfo ti, void[] arr) nothrow
{
    // note, we do not care about shared.  We are setting the length no matter
    // what, so no lock is required.
    ...
    __setArrayAllocLength(info, (arr.ptr - info.base) + cursize, false); //false represents "isshared"
}

Is this comment and implementation accurate? If so, then we can statically split __setArrayAllocLength into __setUnsharedArrayAllocLength, which would be 100% certiiable nothrow, and we'd be able to mark assumeSafeAppend as such too (reliably).

[monarchdodra]

@dawgfoto: What do you think about my above comment. I could statically prove that assumeSafeAppend is nothrow (provided its implementation is correct...). That would just shuffle code around for no real reason though.

An alternative would be to split each and every function in lifetime depending on whether or not it operates on shared or unshared data, and object would statically call the correct function depending data it statically knows.

That would be a much more complex development though, and would really just be a workaround to the whole "druntime has no templates" problem.

So... my conclusion (IMO), is that we should just mark assumeSafeAppend as nothrow, so we can make better use of it in phobos and end user code. At this point, it's silly to have reserve nothrow but not assueSafeAppend.

[monarchdodra]

Reoping per @schveiguy 's request:
http://forum.dlang.org/thread/op.xcn2ebi0eav7ka@stevens-macbook-pro.local?page=2#post-op.xcoel6o5eav7ka:40stevens-macbook-pro.local

[schveiguy]

I think the nothrow attributes inside the runtime are not necessary. When you are under the hood, you have to simply assume things like this. It's why allocating memory is pure and nothrow, even though it's not really.

One might put a /*nothrow*/ on the __arrayShrinkToFit function in lifetime.d to identify it really is nothrow. But I don't know that it's necessary.

@MartinNowak any comments? How does one get the autotester to rerun?

[MartinNowak]

We make mistakes with attributes on high level functions, so I'd like to have more compiler verification in general.
The autotester will rerun when the master or the pull branch change.

[schveiguy]

We make mistakes with attributes on high level functions, so I'd like to have more compiler verification in general.

In this case, it's not so much that we can't verify, in fact we can verify that it MAY throw. But the throw would be due to a condition we know will not happen (the lock throwing) unless the system is in an invalid state.

Likewise, we can allow new int[5] to be pure, even though we KNOW it modifies global state.

These are primitives for which we cannot rely on compiler scrutiny. Marking as much as possible nothrow would be nice, but it would stop at some point.

[monarchdodra]

@schveiguy : I should also mark the function as pure, correct.

One might put a /nothrow/ on the __arrayShrinkToFit function in lifetime.d to identify it really is nothrow. But I don't know that it's necessary.

I could, yes. And document in the code why an exception can't be thrown when it calls __setArrayAllocLength (it's the bool isshared = false that means no lock is requested, so there are no calls to locking functions).

[MartinNowak]

But the throw would be due to a condition we know will not happen (the lock throwing) unless the system is in an invalid state.

Then it should throw an Error, right? So maybe the longterm solution is to make Monitor.lock/unlock nothrow.

[MartinNowak]

It's just important that we remain correct with those annotations, otherwise they become useless.

...as for reserve itself, it can throw in for non-shared code if the postblit throws.

Then reserve needs to be fixed, see
dlang/dmd#3364. It's a template so it can quite easily test whether the postblit throws.

[monarchdodra]

But the throw would be due to a condition we know will not happen (the lock throwing) unless the system is in an invalid state.

Then it should throw an Error, right?

The basic idea is that since assumeSafeAppend "assumes" the append is safe, it also assumes the array is not shared, is simply never (runtime) calls any branch that lock. The lock won't throw on account of not being called.

So maybe the longterm solution is to make Monitor.lock/unlock nothrow.

I have no idea if this is correct or not. I think its not necessary here anyways.

---

In any case @MartinNowak , if you take a quick look at #632, I "proved" it is nothrow. The code is ugly, so I wouldn't pull it, but it is correct.

[MartinNowak]

@schveiguy : I should also mark the function as pure, correct.

Why? It's changing globally visible state (array capacity) and the blkinfo cache is altered too.

In any case @MartinNowak , if you take a quick look at #632, I "proved" it is nothrow. The code is ugly, so I wouldn't pull it, but it is correct.

Fine, it would still be useful to refactor __setArrayAllocLength, first to avoid all the redundant code, second to split it into a throwing and a unshared nothrow version.

[monarchdodra]

Why? It's changing globally visible state (array capacity) and the blkinfo cache is altered too.

For the same reasons that appending to an array is pure (provided no postblit): GC memory management and allocations are considered (weakly) pure. And besides, the function does take a pointer to the array, which gives it the right to modify it (albeit in an underhanded global manner).

Am I completely mistaken?

[schveiguy]

To give some background, assumeSafeAppend does not alter global state. It modifies a field contained within the array memory block. However, to determine the extents of the block the pointer refers to, it must use the heap's metadata to look up the block information for that pointer.

I think in all cases, looking up block info should be nothrow, pure, and safe, even if the underlying functions are not provably so. Otherwise, critical components of the language cannot function properly. If they can be fully marked that way, fine, but only if the requirements on marking the underlying functions don't affect other code.

In the case of altering information on shared blocks, for appending, a global lock is required in case two threads are appending to the same array simultaneously. However, I do not take the lock on altering the size for assumeSafeAppend because I don't care what other threads are doing, I'm going to alter the size anyway. However, thinking about it, that may incorrectly cause races.

[MartinNowak]

Auto-merge toggled on

[monarchdodra]

TYVM @schveiguy for looking into this pull, and backing.

TYVM @MartinNowak for reviewing and pulling into drunime.

[MartinNowak]

I'd still like to encourage anyone to put those kind of attribute decision on a more formal basis so that we preserve the ability to use them for compiler optimizations.
Way to often the argumentation is usability biased, a.k.a. it has to be pure because I want to use it in phobos.

[schveiguy]

@MartinNowak I agree, I think we should push down as far as possible the actual attributes, and create a layer where things aren't provably pure/safe/nothrow, but must be labeled as such in order to get useful work done. Ideally, these would be deep inside druntime. Any real decision on where this should be should really be discussed with the community.

[monarchdodra]

@MartinNowak , @schveiguy there was apparently a problem, that when used with immutable types, assumeSafeAppend is infered as strongly pure.

See here: dlang/phobos#2025

This might be a problem. Strongly pure functions can be optimized out if we don't use the return value, especially if marked nothrow to boot...

[WalterBright]

It's why allocating memory is pure and nothrow, even though it's not really.

Allocating memory via new is definitely nothrow. Running out of memory will throw an Error, but since Errors are not recoverable, such is regarded as nothrow.

Some functions can be legitimatedly converted to nothrow if the exceptions they throw are Error based rather than recoverable Exceptions.