Skip to content

Conversation

@MihaZupan
Copy link
Member

Fixes #79774

this in this case is a struct, and ObjectDisposedException has two overloads

void ThrowIf(bool condition, object instance);
void ThrowIf(bool condition, Type type); 

@MihaZupan MihaZupan added this to the 8.0.0 milestone Dec 17, 2022
@ghost
Copy link

ghost commented Dec 17, 2022

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #79774

this in this case is a struct, and ObjectDisposedException has two overloads

void ThrowIf(bool condition, object instance);
void ThrowIf(bool condition, Type type); 
Author: MihaZupan
Assignees: -
Labels:

area-System.Threading

Milestone: 8.0.0

@ghost ghost assigned MihaZupan Dec 17, 2022
@sebastienros
Copy link
Member

/benchmark json aspnet-citrine-win runtime

@davidfowl
Copy link
Member

I was staring at the original PR trying to figure out what was going on 😄

@davidfowl
Copy link
Member

cc @stephentoub

@pr-benchmarks
Copy link

pr-benchmarks bot commented Dec 17, 2022

Benchmark started for json on aspnet-citrine-win with runtime. Logs: link

@pr-benchmarks
Copy link

pr-benchmarks bot commented Dec 17, 2022

An error occured, please check the logs

@MihaZupan
Copy link
Member Author

error: unable to create file src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.OnReferenceType.NoKeptCtorButInterfaceNeededTests.g.cs: Filename too long

@MihaZupan
Copy link
Member Author

MihaZupan commented Dec 17, 2022

Here you go

crank --application.framework net8.0 --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/platform.benchmarks.yml --scenario json --profile aspnet-citrine-win
application main pr
CPU Usage (%) 83 84 +1,20%
Cores usage (%) 2.311 2.345 +1,47%
Working Set (MB) 434 74 -82,95%
Private Memory (MB) 463 100 -78,40%
Build Time (ms) 2.952 2.930 -0,75%
Start Time (ms) 321 318 -0,93%
Published Size (KB) 97.090 97.090 0,00%
Symbols Size (KB) 45 45 0,00%
.NET Core SDK Version 8.0.100-alpha.1.22616.7 8.0.100-alpha.1.22616.7

@davidfowl
Copy link
Member

BeautifulAkonGIF

@davidfowl
Copy link
Member

davidfowl commented Dec 17, 2022

// Have we been disposed?
ObjectDisposedException.ThrowIf(oldCount < 0, this);
if (oldCount < 0)
throw new ObjectDisposedException(typeof(T).ToString());
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing. We'll want to fix the analyzer/fixer as well to not fire in this case. But that's not pressing and I can do that when I'm back.

Copy link
Member

Choose a reason for hiding this comment

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

We'll want to fix the analyzer/fixer as well to not fire in this case. But that's not pressing and I can do that when I'm back.

dotnet/roslyn-analyzers#6396

@jkotas jkotas merged commit 56e22a4 into dotnet:main Dec 17, 2022
@mangod9
Copy link
Member

mangod9 commented Dec 17, 2022

Whoa, that was a large regression due to a one line change. Are there other places which need to be looked at, since the original change had a lot more changes?

@MihaZupan
Copy link
Member Author

I scanned through the ODE.ThrowIf PR again for places using this, and this was the only one on a struct as far as I could tell.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 2, 2023
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.

Performance regression - Allocation/WorkingSet on PlaintextPlatform

6 participants