On 9/20/18 11:33 AM, Adam D. Ruppe wrote:
> On Wednesday, 19 September 2018 at 21:16:00 UTC, Steven Schveighoffer wrote:
>> As Andrei says -- Destroy!
> 
> Nah, I agree. Actually, I'm of the opinion that string error messages in exceptions ought to be considered harmful: you shouldn't be doing strings at all. All the useful information should be in the type - the class name and the members with details.
> 
> Well, defining a new class can sometimes be a mild hassle... but for really common ones, we really should just do it, and other ones can be done as templated classes or templated factory functions that define a new class right there and then.
> 
> http://arsdnet.net/dcode/exception.d
> 
> That's the proof-of-concept I wrote for this years ago, go to the bottom of the file for the usage example. It uses a reflection mixin to make writing the new classes easy, and I even wrote an enforce thing that can add more info by creating a subclass that stores arguments to functions so it can print it all (assuming they are sane to copy like strings or value things lol)
> 
>      enforce!fopen("nofile.txt".ptr, "rb".ptr);
> 
> MyExceptionBase@exception.d(38): fopen call failed
>          filename = nofile.txt
>          mode = rb
> ----------------


Awesome! This is just what I was thinking of. In fact, I did something similar locally since I needed to know what the slice parameters that were failing were. I still had to trick the @nogc to get around the "new Exception" piece.

The printMembers thing is nice. I think for druntime/phobos, however, we should have a base that just calls a virtual function with the idea that the message is printed, and then a further-derived type could do the printMembers thing if that's what you want.

-Steve

On Thu, Sep 20, 2018 at 11:25:27AM -0400, Steven Schveighoffer via Digitalmars-d wrote:
> On 9/20/18 11:06 AM, H. S. Teoh wrote:
[...]
> > IIRC, originally the stacktrace was also built at exception construction time. But it was causing a major performance hit, so eventually someone changed the code to construct it lazily (i.e., only when the catcher actually tries to look it up).
> > 
> > I think it makes sense to also make .msg lazy, if the exception object is already carrying enough info to build the message when the catcher asks for it. And if the catcher doesn't ask for it, we saved an extra GC allocation (which is a plus even if we're not trying to go @nogc).
> 
> Except we DON'T construct the stack trace string, even lazily. If you look at the code I posted, it's output directly to the output buffer (via the sink delegate), without ever having allocated.
> 
> I think we can do that for the message too (why not, it's all supported).  But either one (using GC at print time, or lazily outputting to buffer at print time) solves the biggest problem -- being able to construct an exception without the GC.
[...]

Yeah, that's what I meant. :D  Well, for backward compatibility we could still have .msg allocate and return a string, but we could provide an overload / alternate member function that writes directly to a sink instead.  Then we don't ever have to allocate unless the sink itself does.


T

-- 
I am a consultant. My job is to make your job redundant. -- Mr Tom

On Thursday, 20 September 2018 at 15:52:03 UTC, Steven Schveighoffer wrote:
> I needed to know what the slice parameters that were failing were.

Aye. Note that RangeError is called by the compiler though, so you gotta patch dmd to make it pass the arguments to it for index. Ugh. I did a PR for this once but it got shot down because of an allegeded (without evidence btw) performance degradation. Ugh.

> The printMembers thing is nice. I think for druntime/phobos, however, we should have a base that just calls a virtual function with the idea that the message is printed

Aye.

On 9/20/18 12:24 PM, Adam D. Ruppe wrote:
> On Thursday, 20 September 2018 at 15:52:03 UTC, Steven Schveighoffer wrote:
>> I needed to know what the slice parameters that were failing were.
> 
> Aye. Note that RangeError is called by the compiler though, so you gotta patch dmd to make it pass the arguments to it for index. Ugh. I did a PR for this once but it got shot down because of an allegeded (without evidence btw) performance degradation. Ugh.

Well, you can always override that. Just do the check yourself and throw the error you want ;)

In my case, that's what I did anyway.

I don't know how a performance problem can occur on an error being thrown anyway -- the process is about to end.

-Steve

On Thursday, 20 September 2018 at 17:14:12 UTC, Steven Schveighoffer wrote:
> I don't know how a performance problem can occur on an error being thrown anyway -- the process is about to end.

Walter's objection was code size - it would throw stuff out of cache lines, even if it doesn't need to actually run.

So like this line:

int[] a;
a = a[1 .. $];

With no bounds checking is just

inc a.ptr;

but with bounds checking it becomes something more like

mov ecx, a.length
cmp ecx, 1 // in other words, if length >= offset
jae proceed
push line
push file
call d_arraybounds // throws the error
proceed:
inc a.ptr


Now, what my patch did was just, right before push line, it inserted "push length; push offset;". I believe this to be trivial since they are already loaded in registers or immediates and thus just a couple bytes for those instructions, but Walter (as I recall, this was a while ago and I didn't look up his exact words when writing this) said even a couple bytes are important for such a common operation as it throws off the L1 caches. I never got around to actually measuring the performance impact to prove one way or another.

But... even if that is a problem, dmd -O will usually rearrange that to avoid the jump on the in-bounds case, and I'm sure ldc/gdc do too, sooooo the extra pushes' instruction bytes are off the main execution path anyway and thus shouldn't waste cache space.


idk though. regardless, to me, the extra info is *well* worth the cost anyway.

On Thursday, 20 September 2018 at 15:52:15 UTC, H. S. Teoh wrote:
> Yeah, that's what I meant. :D  Well, for backward compatibility we could still have .msg allocate and return a string, but we could provide an overload / alternate member function that writes directly to a sink instead.  Then we don't ever have to allocate unless the sink itself does.

I believe Tango did this a decade ago. It's a solid strategy. However, with Tango, the default was to implement the toString(sink) function by calling the regular toString(), which was quite convenient but won't work with @nogc.

On 9/20/18 1:58 PM, Adam D. Ruppe wrote:
> On Thursday, 20 September 2018 at 17:14:12 UTC, Steven Schveighoffer wrote:
>> I don't know how a performance problem can occur on an error being thrown anyway -- the process is about to end.
> 
> Walter's objection was code size - it would throw stuff out of cache lines, even if it doesn't need to actually run.
> 
> So like this line:
> 
> int[] a;
> a = a[1 .. $];
> 
> With no bounds checking is just
> 
> inc a.ptr;
> 
> but with bounds checking it becomes something more like
> 
> mov ecx, a.length
> cmp ecx, 1 // in other words, if length >= offset
> jae proceed
> push line
> push file
> call d_arraybounds // throws the error
> proceed:
> inc a.ptr
> 
> 
> Now, what my patch did was just, right before push line, it inserted "push length; push offset;". I believe this to be trivial since they are already loaded in registers or immediates and thus just a couple bytes for those instructions, but Walter (as I recall, this was a while ago and I didn't look up his exact words when writing this) said even a couple bytes are important for such a common operation as it throws off the L1 caches. I never got around to actually measuring the performance impact to prove one way or another.
> 
> But... even if that is a problem, dmd -O will usually rearrange that to avoid the jump on the in-bounds case, and I'm sure ldc/gdc do too, sooooo the extra pushes' instruction bytes are off the main execution path anyway and thus shouldn't waste cache space.
> 
> 
> idk though. regardless, to me, the extra info is *well* worth the cost anyway.

Sounds like a case of premature optimization at best.

Besides, if it is a performance issue, you aren't doing bounds checks on every slice/index anyway. I know in iopipe, to squeeze out every bit of performance, I avoid bounds checks when I know from previous asserts the bounds are correct.

-Steve

On Wednesday, September 19, 2018 3:16:00 PM MDT Steven Schveighoffer via Digitalmars-d wrote:
> Given dip1008, we now can throw exceptions inside @nogc code! This is really cool, and helps make code that uses exceptions or errors @nogc. Except...

I pointed out this problem when the DIP was orginally proposed and have always thought that it was kind of pointless because of this issue. The core problem is that Exception predates @nogc. It predates ranges. Its design makes sense if exceptions are rare, and you're willing to use the GC. IMHO, it even makes sense in most cases with most code that is trying to avoid allocations, because the allocations are only going to occur when an error condition occurs. So, for most code bases, they won't affect performance. The problem is that even though that usually makes perfect sense, if you've written your code so that the non-error path doesn't allocate, you'd really like to be able to mark it with @nogc, and you can't because of the exceptions. The DIP helps but not enough to really matter. And of course, in those code bases where you actually can't afford to have the error path allocate, it's even more of a problem.

So, what we have is an Exception class that works perfectly well in a world without @nogc but which doesn't work with @nogc worth anything. And if we want to fix that, I think that we need to figure out how to fix Exception in a way that we can sanely transitition to whatever the new way we handle exception messages would be. I think that the message member was added by Dicebot as an attempt to fix this issue, because Sociomantic needed it, but I don't know exactly what's going on with that or @__future.

- Jonathan M Davis

On Friday, 21 September 2018 at 09:10:06 UTC, Jonathan M Davis wrote:
> [...]
> I think that the message member was added by Dicebot as an attempt to fix this issue, because Sociomantic needed it, but I don't know exactly what's going on with that or @__future.
>
> - Jonathan M Davis

The @__future is fully (to a reasonable degree) implemented - and the `Throwable.message` was marked with this attribute to prevent breaking the user code when introducing this field, and it probably can just be removed from there at this point (since many releases have been released).

This is the PR when Throwable.message was first introduced: https://github.com/dlang/druntime/pull/1445 We had the same discussion there, and you can see sociomantic's use case in Mihail's comments there. The entire PR have a very good discussion and we (not Sociomantic) still need sink-based method IIUC.

On Friday, 21 September 2018 at 10:06:06 UTC, Nemanja Boric wrote:
> On Friday, 21 September 2018 at 09:10:06 UTC, Jonathan M Davis wrote:
>> [...]
>
> The @__future is fully (to a reasonable degree) implemented - and the `Throwable.message` was marked with this attribute to prevent breaking the user code when introducing this field, and it probably can just be removed from there at this point (since many releases have been released).
>
> [...]

Interesting quote from Martin on that PR:

> With regards to Throwable.message we agreed on making it one of the first users of an upcoming reference counted string. Details on the design of the reference counted string first to be discussed on dlang-study.