fix issue 6333

[sandford]

Patch for Issue 6333 - The 'capacity' function is not pure/nothrow/@safe.
http://d.puremagic.com/issues/show_bug.cgi?id=6333

Adds a new _d_arraygetcapacity function which is copy / pasted / streamlined from _d_arraysetcapacity. Both _d_arraygetcapacity and _d_arraysetcapacity now always update the block cache if it is okay to do so and the info block wasn't already present.

nothrow has been added to the definitions of the get/set functions and their dependents with the exception of the raw GC calls.

pure is enabled by a hack in object.di and object_.d which defines the extern(C) as being pure. (This same trick is used to cast the GC as pure nothrow inside lifetime.d). Technically, neither of these routines are strictly pure, but then again, neither is new T[n].

[yebblies]

Could you please clean up the commits for this? Using git rebase -i and squash should do the trick.

[schveiguy]

Hm... this looks like it will fail if you try to reserve or use capacity on a shared array. I think if you remove the pure nothrow attributes from the template, it should become automatically pure/nothrow depending on the arguments passed.

EDIT: I guess just removing pure should be fine, let the complier automatically decide whether it's pure or not.

[schveiguy]

Also, some additional unit tests to show that it can be used in pure functions (and on shared arrays, even though I should have put them in originally...) would be good.

[schveiguy]

Can you explain what the benefit is for splitting arraysetcapacity into two functions?

[schveiguy]

I don't like the idea of adding to the cache on checking capacity. The cache is strictly for appending. If you are not appending, or extending the length, the cache should be left alone.

This could be a good reason to split out the functions :)

[kennytm]

What's the difference between this and #147?

[schveiguy]

Oh right, I forgot completely about that.

One major difference (but is easily fixed in #147) is I didn't think about whether shared arrays could have capacity/reserve called on them. Another major difference is the splitting of the functions, but I'm not sure that's necessary yet.

[sandford]

let the complier automatically decide whether it's pure or not

To the best of my knowledge, the compiler can not automatically decide whether it's pure or not (halting problem). That said, I'm starting to feel the need for conditional function attributes. (i.e. I'm pure if my caller is pure, or Func is pure, etc)

The cache is strictly for appending.

How often does one check the capacity of an array and then not call reserve/set the length/append?
There isn't much data on this in Phobos but the only two uses of capacity are:
if(app.capacity == 0)
app.reserve(128); // get at least 128 bytes available
and
auto cap = arr.capacity;
if(cap > arr.length)
arr.length = cap;

Can you explain what the benefit is for splitting arraysetcapacity into two functions?

In retrospect, fairly little. arraygetcapacity is a more streamlined and optimized (i.e. faster) but I don't know if that's worth the code duplication. In design, capacity is honestly nothrow while reserve can throw out of memory errors. In coding, I had thought getting a stripped down function working as pure nothrow would be easier. Doing so had the side benefit catch a Issue 7523 :)

What's the difference between this and #147?

Minor differences: I also added @trusted to capacity / reserve and nothrow attributes to various parts of lifetime.d which might improve the generated code.

[schveiguy]

1. I think the compiler will currently automatically make a template function instantiation pure if it could be pure. i.e.

   pure void foo() {}
   auto bar(alias fn)() { return fn();}
   pure void baz() { bar!foo();}

2. I don't know how often one calls capacity on an array and not set the length or append, but the cache is supposed to be for frequent appending/extending. Using up a cache space for an individual call to capacity is somewhat wasteful. That first call seems nonsensical. If you reserve if it's zero, why not just reserve? The second call is a legitimate reason, but I argue there should really be a function in druntime that extends an array to it's full capacity (only do one blockinfo lookup). I've frequently used that idiom.

3. Yes, maintaining two nearly identical functions needs to show a compelling performance gain. I already don't like the duplication of functionality in lifetime.d, but the calls are dictated by the compiler. Note that memory errors are Errors, nothrow means Exceptions, so anything that throws an out of memory error is still legitimately nothrow.

4. I agree with adding @trusted to capacity/reserve, and the added nothrows.

[sandford]

1. You're correct :)
2. Extending an array to its full capacity seems like the actual function we want; why else access capacity?
3. I agree that the drawbacks of a second function outweigh the benefits.
4. Strangely, as currently written, TLS and global arrays work and shared arrays don't.

[schveiguy]

Accessing capacity can help you make a decision on whether to call assumeSafeAppend or not. a non-zero capacity means there is no valid data in the array after the given slice.

[sandford]

So something like:

if(arr.capacity > 0) {
    arr.length = 0;
    arr.assumeSafeAppend;
} else { ... }

Personally I think

appender(arr).clear;

would probably be the better design choice, but I understand the need.

[sandford]

I updated the patch with some unittests and removed the duplicate function and the extra cache updates. I also updated reserve to work with shared arrays; capacity worked as is. (which may be a DMD bug, but that's a separate issue.)

[jmdavis]

This does not currently compile. I get

src/rt/lifetime.d(698): Error: gc_extend is not nothrow
src/rt/lifetime.d(712): Error: gc_qalloc is not nothrow
src/rt/lifetime.d(721): Error: __doPostblit is not nothrow
src/rt/lifetime.d(736): Error: __setArrayAllocLength is not nothrow
src/rt/lifetime.d(756): Error: onOutOfMemoryError is not nothrow
src/rt/lifetime.d(610): Error: function rt.lifetime._d_arraysetcapacity '_d_arraysetcapacity' is nothrow yet may throw

when I try and compile it on 64-bit Linux.

[jmdavis]

And from the looks of it, you're going to have to mark everything and its grandmother in the GC code nothrow in order to get this to work.

[schveiguy]

We can probably get around this by marking the prototypes nothrow, they are extern(C).

But in the end, we really should mark all the GC functions nothrow.

[sandford]

Thank you, jmdavis, for testing the patch. I don't have time this weekend to look into the issue in detail, but will do so on Monday. If I might ask, where you using DMD 2.058 or 2.059 beta?

[jmdavis]

@sandford I used the latest from head (that's what always should be used when submitting and testing pull requests, because that's what they get merged into) and that should match the current beta.

[sandford]

@jmdavis Well, the head is always moving :) That said, I've updated to today's 2.059 beta and the head revisions of druntime and phobos. And it would appear that despite these functions not being marked nothrow, everything compiles on Win32. This is probably a compiler bug of some kind. I went ahead and added pure and nothrow to all of the GC function prototypes in lifetime.d. The pull request has been updated.

[jmdavis]

I just tried to compile this again on my 64-bit Linux box, and I get this

src/rt/lifetime.d(721): Error: __doPostblit is not nothrow
src/rt/lifetime.d(736): Error: __setArrayAllocLength is not nothrow
src/rt/lifetime.d(610): Error: function rt.lifetime._d_arraysetcapacity '_d_arraysetcapacity' is nothrow yet may throw

MODEL=32 compiles fine, but MODEL=64 doesn't. So, there are now fewer compilation errors there there were before, but there are still compilation errors. And looking at the pull tester, it looks like this pull request is failing on all platforms. On the 64-bit plaftorms, it's failing due to the errors above, and on everything else, it's failing because it causes dmd's tests to fail.

[sandford]

Thank you. I've only been testing with the phobos and druntime unittests, but I guess I should also get a more extensive test environment setup as well as a 64-bit platform (since it is behaving substantially differently). For now, I think I'll close this pull request until its ready on all systems.