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

STJ: Add AggressiveInlining to CheckNotDisposed() #71534

Merged
merged 3 commits into from
Jul 1, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jul 1, 2022

Improves performance for the #67176 benchmark (it regressed due to a different reason, but this looks like a low-hanging fruit and other workloads/benchmarks will benefit from this as well)

private void CheckNotDisposed()
{
    if (_utf8Json.IsEmpty)
    {
        throw new ObjectDisposedException(nameof(JsonDocument));
    }
}

Currently, JIT rarely inlines methods with throw inside (mostly only when it sees foldable branches, etc.) because it knows that they usually come with a lot of size increase for the caller. Technically, it's not a big deal since such throw related pieces will be marked as cold and won't be involved. However, I think we need to wait for "partial compilation" feature in the JIT to be less conservative (because throw-blocks won't be even compiled) here or otherwise will see a huge positive jit-diffs.

@ghost
Copy link

ghost commented Jul 1, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Improves performance for the #67176 benchmark (it regressed due to a different reason, but this looks like a low-hanging fruit and other workloads/benchmarks will benefit from this as well)

private void CheckNotDisposed()
{
    if (_utf8Json.IsEmpty)
    {
        throw new ObjectDisposedException(nameof(JsonDocument));
    }
}

Currently, JIT rarely inlines methods with throw inside (mostly only when it sees foldable branches, etc.) because it knows that they usually come with a lot of size increase for the caller. Technically, it's not a big deal since such throw related pieces will be marked as cold and won't be involved. However, I think we need to wait for "partial compilation" feature in the JIT to be less conservative (because throw-blocks won't be even compiled) here or otherwise will see a huge positive jit-diffs.

Author: EgorBo
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks!

@AndyAyersMS
Copy link
Member

However, I think we need to wait for "partial compilation" feature in the JIT to be less conservative (because throw-blocks won't be even compiled) here or otherwise will see a huge positive jit-diffs.

The fundamental blocker here is making partial compilation work when we're optimizing code. For unoptimized code it's not hard to figure out where all the continuation state is located since Tier0 codegen is very simplistic, but for optimized code....

But in cases like this where there is no live continuation state (that is, if the throw does not refer to any locals), perhaps it would be not too hard even when optimizing.

@EgorBo EgorBo merged commit abc028d into dotnet:main Jul 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants