Analyze cibuild hang on Resharper tools update #1045
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bart-degreed
force-pushed
the
jetbrains-update
branch
from
ea17d1f to
eb42f23
Compare
bart-degreed
pushed a commit
that referenced
this pull request
Nov 2, 2021
In the past, [cibuild hung after update to the latest version](#1045). Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging. Stats from my laptop (old=v2021.1.4, new=v2021.2.2): ``` master, inspectcode (old): 2:59 master, inspectcode (new): 2:00 nrt, inspectcode (old): 3:04 nrt, inspectcode (new): 2:33 master, cleanupcode (old): 11:06 master, cleanupcode (new): 5:39 nrt, cleanupcode (old): 11:59 nrt, cleanupcode (new): 6:34 ``` So the newer versions got faster. And cleanupcode still takes the most time. In this PR, 722 of 1020 files have changed, which is 70% of the total codebase. Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out. This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward.
bart-degreed
pushed a commit
that referenced
this pull request
Nov 3, 2021
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls. * Added directive to disable NRT in all source files * Turn on NRT globally * Annotated JsonApiDotNetCore library * Use alternate public name for attribute, to improve test coverage * Annotated example projects * Annotated benchmark project - Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore - Cleanup query string benchmark * Enhanced rendering of error.source.pointer on ModelState validation errors * Respect configured MaxModelValidationErrors in operations * Added docs for nullable reference types * Automated cleanup code * Fixed: do not fail when clearing a required to-many relationship * Annotated controllers in integration tests * Annotated DbContexts in integration tests * Annotated fakers in integration tests * Annotated TestBuildingBlocks * Use Should() replacements that flow nullability * Re-align similar testsets * Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere * Fixed: make faker dates deterministic See https://giters.com/bchavez/Bogus/issues/247 * Enable ASP.NET ModelState validation by default Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations. * Check off roadmap entry * Documented required one-to-one relationships mapping * Added missing HEAD routes * Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup * Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph. | Method | Mean | Error | StdDev | | ------------------------------------ | -------- | ------- | ------- | | SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us | | SerializeOperationsResponse (after) | 148.5 us | 1.06 us | 0.99 us | | SerializeResourceResponse (before) | 123.7 us | 1.12 us | 1.05 us | | SerializeResourceResponse (after) | 119.6 us | 1.05 us | 0.98 us | This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher. * Fixed: handle nulls in request body * Workaround for crash in cibuild * Update to latest JetBrains tools In the past, [cibuild hung after update to the latest version](#1045). Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging. Stats from my laptop (old=v2021.1.4, new=v2021.2.2): ``` master, inspectcode (old): 2:59 master, inspectcode (new): 2:00 nrt, inspectcode (old): 3:04 nrt, inspectcode (new): 2:33 master, cleanupcode (old): 11:06 master, cleanupcode (new): 5:39 nrt, cleanupcode (old): 11:59 nrt, cleanupcode (new): 6:34 ``` So the newer versions got faster. And cleanupcode still takes the most time. In this PR, 722 of 1020 files have changed, which is 70% of the total codebase. Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out. This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward. * Package updates * Review non-public TryXXX methods * Addressed review feedback
bart-degreed
pushed a commit
that referenced
this pull request
Dec 3, 2021
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls. * Added directive to disable NRT in all source files * Turn on NRT globally * Annotated JsonApiDotNetCore library * Use alternate public name for attribute, to improve test coverage * Annotated example projects * Annotated benchmark project - Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore - Cleanup query string benchmark * Enhanced rendering of error.source.pointer on ModelState validation errors * Respect configured MaxModelValidationErrors in operations * Added docs for nullable reference types * Automated cleanup code * Fixed: do not fail when clearing a required to-many relationship * Annotated controllers in integration tests * Annotated DbContexts in integration tests * Annotated fakers in integration tests * Annotated TestBuildingBlocks * Use Should() replacements that flow nullability * Re-align similar testsets * Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere * Fixed: make faker dates deterministic See https://giters.com/bchavez/Bogus/issues/247 * Enable ASP.NET ModelState validation by default Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations. * Check off roadmap entry * Documented required one-to-one relationships mapping * Added missing HEAD routes * Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup * Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph. | Method | Mean | Error | StdDev | | ------------------------------------ | -------- | ------- | ------- | | SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us | | SerializeOperationsResponse (after) | 148.5 us | 1.06 us | 0.99 us | | SerializeResourceResponse (before) | 123.7 us | 1.12 us | 1.05 us | | SerializeResourceResponse (after) | 119.6 us | 1.05 us | 0.98 us | This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher. * Fixed: handle nulls in request body * Workaround for crash in cibuild * Update to latest JetBrains tools In the past, [cibuild hung after update to the latest version](#1045). Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging. Stats from my laptop (old=v2021.1.4, new=v2021.2.2): ``` master, inspectcode (old): 2:59 master, inspectcode (new): 2:00 nrt, inspectcode (old): 3:04 nrt, inspectcode (new): 2:33 master, cleanupcode (old): 11:06 master, cleanupcode (new): 5:39 nrt, cleanupcode (old): 11:59 nrt, cleanupcode (new): 6:34 ``` So the newer versions got faster. And cleanupcode still takes the most time. In this PR, 722 of 1020 files have changed, which is 70% of the total codebase. Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out. This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward. * Package updates * Review non-public TryXXX methods * Addressed review feedback
bart-degreed
pushed a commit
that referenced
this pull request
Dec 3, 2021
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls. * Added directive to disable NRT in all source files * Turn on NRT globally * Annotated JsonApiDotNetCore library * Use alternate public name for attribute, to improve test coverage * Annotated example projects * Annotated benchmark project - Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore - Cleanup query string benchmark * Enhanced rendering of error.source.pointer on ModelState validation errors * Respect configured MaxModelValidationErrors in operations * Added docs for nullable reference types * Automated cleanup code * Fixed: do not fail when clearing a required to-many relationship * Annotated controllers in integration tests * Annotated DbContexts in integration tests * Annotated fakers in integration tests * Annotated TestBuildingBlocks * Use Should() replacements that flow nullability * Re-align similar testsets * Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere * Fixed: make faker dates deterministic See https://giters.com/bchavez/Bogus/issues/247 * Enable ASP.NET ModelState validation by default Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations. * Check off roadmap entry * Documented required one-to-one relationships mapping * Added missing HEAD routes * Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup * Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph. | Method | Mean | Error | StdDev | | ------------------------------------ | -------- | ------- | ------- | | SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us | | SerializeOperationsResponse (after) | 148.5 us | 1.06 us | 0.99 us | | SerializeResourceResponse (before) | 123.7 us | 1.12 us | 1.05 us | | SerializeResourceResponse (after) | 119.6 us | 1.05 us | 0.98 us | This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher. * Fixed: handle nulls in request body * Workaround for crash in cibuild * Update to latest JetBrains tools In the past, [cibuild hung after update to the latest version](#1045). Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging. Stats from my laptop (old=v2021.1.4, new=v2021.2.2): ``` master, inspectcode (old): 2:59 master, inspectcode (new): 2:00 nrt, inspectcode (old): 3:04 nrt, inspectcode (new): 2:33 master, cleanupcode (old): 11:06 master, cleanupcode (new): 5:39 nrt, cleanupcode (old): 11:59 nrt, cleanupcode (new): 6:34 ``` So the newer versions got faster. And cleanupcode still takes the most time. In this PR, 722 of 1020 files have changed, which is 70% of the total codebase. Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out. This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward. * Package updates * Review non-public TryXXX methods * Addressed review feedback
bart-degreed
pushed a commit
that referenced
this pull request
Dec 3, 2021
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls. * Added directive to disable NRT in all source files * Turn on NRT globally * Annotated JsonApiDotNetCore library * Use alternate public name for attribute, to improve test coverage * Annotated example projects * Annotated benchmark project - Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore - Cleanup query string benchmark * Enhanced rendering of error.source.pointer on ModelState validation errors * Respect configured MaxModelValidationErrors in operations * Added docs for nullable reference types * Automated cleanup code * Fixed: do not fail when clearing a required to-many relationship * Annotated controllers in integration tests * Annotated DbContexts in integration tests * Annotated fakers in integration tests * Annotated TestBuildingBlocks * Use Should() replacements that flow nullability * Re-align similar testsets * Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere * Fixed: make faker dates deterministic See https://giters.com/bchavez/Bogus/issues/247 * Enable ASP.NET ModelState validation by default Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations. * Check off roadmap entry * Documented required one-to-one relationships mapping * Added missing HEAD routes * Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup * Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph. | Method | Mean | Error | StdDev | | ------------------------------------ | -------- | ------- | ------- | | SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us | | SerializeOperationsResponse (after) | 148.5 us | 1.06 us | 0.99 us | | SerializeResourceResponse (before) | 123.7 us | 1.12 us | 1.05 us | | SerializeResourceResponse (after) | 119.6 us | 1.05 us | 0.98 us | This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher. * Fixed: handle nulls in request body * Workaround for crash in cibuild * Update to latest JetBrains tools In the past, [cibuild hung after update to the latest version](#1045). Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging. Stats from my laptop (old=v2021.1.4, new=v2021.2.2): ``` master, inspectcode (old): 2:59 master, inspectcode (new): 2:00 nrt, inspectcode (old): 3:04 nrt, inspectcode (new): 2:33 master, cleanupcode (old): 11:06 master, cleanupcode (new): 5:39 nrt, cleanupcode (old): 11:59 nrt, cleanupcode (new): 6:34 ``` So the newer versions got faster. And cleanupcode still takes the most time. In this PR, 722 of 1020 files have changed, which is 70% of the total codebase. Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out. This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward. * Package updates * Review non-public TryXXX methods * Addressed review feedback
nicolestandifer3
added a commit
to nicolestandifer3/DotNet-Core-Json-Api
that referenced
this pull request
Aug 6, 2023
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls. * Added directive to disable NRT in all source files * Turn on NRT globally * Annotated JsonApiDotNetCore library * Use alternate public name for attribute, to improve test coverage * Annotated example projects * Annotated benchmark project - Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore - Cleanup query string benchmark * Enhanced rendering of error.source.pointer on ModelState validation errors * Respect configured MaxModelValidationErrors in operations * Added docs for nullable reference types * Automated cleanup code * Fixed: do not fail when clearing a required to-many relationship * Annotated controllers in integration tests * Annotated DbContexts in integration tests * Annotated fakers in integration tests * Annotated TestBuildingBlocks * Use Should() replacements that flow nullability * Re-align similar testsets * Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere * Fixed: make faker dates deterministic See https://giters.com/bchavez/Bogus/issues/247 * Enable ASP.NET ModelState validation by default Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations. * Check off roadmap entry * Documented required one-to-one relationships mapping * Added missing HEAD routes * Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup * Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph. | Method | Mean | Error | StdDev | | ------------------------------------ | -------- | ------- | ------- | | SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us | | SerializeOperationsResponse (after) | 148.5 us | 1.06 us | 0.99 us | | SerializeResourceResponse (before) | 123.7 us | 1.12 us | 1.05 us | | SerializeResourceResponse (after) | 119.6 us | 1.05 us | 0.98 us | This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher. * Fixed: handle nulls in request body * Workaround for crash in cibuild * Update to latest JetBrains tools In the past, [cibuild hung after update to the latest version](json-api-dotnet/JsonApiDotNetCore#1045). Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging. Stats from my laptop (old=v2021.1.4, new=v2021.2.2): ``` master, inspectcode (old): 2:59 master, inspectcode (new): 2:00 nrt, inspectcode (old): 3:04 nrt, inspectcode (new): 2:33 master, cleanupcode (old): 11:06 master, cleanupcode (new): 5:39 nrt, cleanupcode (old): 11:59 nrt, cleanupcode (new): 6:34 ``` So the newer versions got faster. And cleanupcode still takes the most time. In this PR, 722 of 1020 files have changed, which is 70% of the total codebase. Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out. This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward. * Package updates * Review non-public TryXXX methods * Addressed review feedback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Created this PR to diagnose the cibuild hangs described at https://youtrack.jetbrains.com/issue/RSRP-481371#focus=Comments-27-5106579.0-0.