Skip to content

Optimization: Don't generate try-catch for nothrow postblits#8184

Merged
RazvanN7 merged 3 commits intodlang:masterfrom
JinShil:postblit_nothow_optimization
Apr 28, 2018
Merged

Optimization: Don't generate try-catch for nothrow postblits#8184
RazvanN7 merged 3 commits intodlang:masterfrom
JinShil:postblit_nothow_optimization

Conversation

@JinShil
Copy link
Copy Markdown
Contributor

@JinShil JinShil commented Apr 18, 2018

While researching Issue 18493 - [betterC] Can't use aggregated type with postblit , I discovered that when the postblit is generated for an aggregate with 2 or more fields, the compiler adds try-catches around all but the first field postblit to ensure that if an exception is thrown in subsequent postblits the destructors are called for preceding successful postblits.

In other words, for this code...

struct S
{
    this(this) nothrow { }
    ~this() {  }
}

struct C
{
    S s1;
    S s2;
}

... the postblit for C looks like this:

this.s1.__postblit();
try
{
    this.s2.__postblit();
}
catch(Throwable __o4)
{
    this.s1.~this();
    throw __o4;
}

I believe this is unnecessary if the field's postblit is attributed with nothrow.

With the change in this PR, the code above will produce a postblit without the try-catch block like that shown below:

this.s1.__postblit();
this.s2.__postblit(); 

This will also allow users of -betterC to attribute their postblits to avoid the error in Issue 18493,

It may be appropriate to omit the try-catches entirely for -betterC builds and runtimes without Throwable declared, but I will leave that to a future pull request depending on how this PR goes.

@dlang-bot
Copy link
Copy Markdown
Contributor

dlang-bot commented Apr 18, 2018

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close Bugzilla Severity Description
18493 blocker [betterC] Can't use aggregated type with postblit

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8184"

@JinShil
Copy link
Copy Markdown
Contributor Author

JinShil commented Apr 18, 2018

I'm also not sure how to make a proper test for this. I would appreciate some suggestions.

@JinShil
Copy link
Copy Markdown
Contributor Author

JinShil commented Apr 18, 2018

My first attempt failed to consider a mixture of fields with nothrow and nonnothrow postblits. Now, the following code...

struct S_nothrow
{
    this(this) nothrow { }

    ~this() { }
}

struct S_throw
{
    this(this) { }

    ~this() { }
}

struct C
{
    S_nothrow s1;
    S_throw s2;
    S_throw s3;
    S_nothrow s4;
    S_throw s5;
}

... will produce a field postblit for C as...

this.s1.__postblit();
try
{
    this.s2.__postblit();
    try
    {
        this.s3.__postblit();
        this.s4.__postblit();
        try
        {
            this.s5.__postblit();
        }
        catch(Throwable __o7)
        {
            this.s4.~this();
            this.s3.~this();
            throw __o7;
        }
    }
    catch(Throwable __o6)
    {
        this.s2.~this();
        throw __o6;
    }
}
catch(Throwable __o5)
{
    this.s1.~this();
    throw __o5;
}

// if this field's postblit is not `nothrow`, add a `scope(failure)`
// block to destroy any prior successfully postblitted fields should
// this field's postblit fail
if (fieldsToDestroy.length > 0 && !(cast(TypeFunction)sdv.postblit.type).isnothrow)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this should actually use the canThrow function, but I can't figure out how to make a proper call to that function. Should I use canThrow here, and if so, how should I construct the call?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

canThrow is used to check whether an expression can throw, while you have a function which needs to be checked. You could use the constructed call expression to the postblit to see if it's marked as nothrow, but that does essentially the same thing that you are doing here [1], so the current check is fine.

[1]

if (t.ty == Tfunction && (cast(TypeFunction)t).isnothrow)

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 23, 2018
@JinShil
Copy link
Copy Markdown
Contributor Author

JinShil commented Apr 24, 2018

Sorry for the late addition. It just occurred to me that I should add a test for Issue 18493, as with this PR users will at least be able to avoid the error by attributing their postblits with nothrow.

I'm currently in a private e-mail discussion with @WalterBright and @andralex about whether to assume nothrow for any function compiled under -betterC, or to emit an error for any function that is not attributed with nothrow when compiled under -betterC. I'll submit a future pull request when that decision is made and we can then close the issue.

@RazvanN7 RazvanN7 merged commit b7f8794 into dlang:master Apr 28, 2018
@JinShil JinShil deleted the postblit_nothow_optimization branch May 28, 2018 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merge:72h no objection -> merge The PR will be merged if there are no objections raised.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants