Fix assert by only accessing idx #6924
Merged
Conversation
This file contains 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
Asserting on `_rowCount < Utils.Size(_valueBoundaries)` was catching a case where `_rowCount`'s update was reordered before `_valueBoundaries` This was unnecessary, since this method doesn't need to use `_rowCount`. Instead, make the asserts use only `idx` which will be maintained consistent with the waiter logic in this cache. �Ensure we only ever use `_rowCount` from the caching thread, so write reordering won't matter.
ghost
assigned 
|
I tested this fix in #6922 as well |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6924 +/- ##
=======================================
Coverage 68.79% 68.79%
=======================================
Files 1249 1249
Lines 249431 249433 +2
Branches 25510 25509 -1
=======================================
+ Hits 171604 171607 +3
- Misses 71214 71216 +2
+ Partials 6613 6610 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
This seems to have fixed things, but now our failures are all timeouts. Hitting here and in #6923. @michaelgsharp didn't you mention wanting to address the timeouts by splitting up the test assemblies? |
michaelgsharp
approved these changes
Dec 22, 2023
|
@ericstj Yeah, as soon as I finish the NER issue I'll test splitting the assemblies, so I'll have a test out for that later today. |
ericstj
added a commit
that referenced
this pull request
Jan 5, 2024
Asserting on `_rowCount < Utils.Size(_valueBoundaries)` was catching a case where `_rowCount`'s update was reordered before `_valueBoundaries` This was unnecessary, since this method doesn't need to use `_rowCount`. Instead, make the asserts use only `idx` which will be maintained consistent with the waiter logic in this cache. �Ensure we only ever use `_rowCount` from the caching thread, so write reordering won't matter.
michaelgsharp
added a commit
that referenced
this pull request
Jan 9, 2024
* Update dependencies from https://github.com/dotnet/arcade build 20231220.2 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.SwaggerGenerator.MSBuild , Microsoft.DotNet.XUnitExtensions From Version 8.0.0-beta.23265.1 -> To Version 8.0.0-beta.23620.2 * Fixed version update breaks. * Update XUnitVersion * Update MicrosoftMLOnnxRuntimeVersion to 1.16.3 * Rollback OnnxRuntime and suppress warning * Update to Xunit with fix for xunit/xunit#2821 * Update Centos docker containers * Fix packaging step * Try including stdint.h to fix missing uint8_t on centos * Update Centos test queue * Attempt to use runtime centos-stream8-helix container for tests * Use centos-stream8-mlnet-helix container for testing * Undo changes to test data * Make NETFRAMEWORK ifdef versionless * Only use semi-colons for NoWarn * Fix assert by only accessing idx (#6924) Asserting on `_rowCount < Utils.Size(_valueBoundaries)` was catching a case where `_rowCount`'s update was reordered before `_valueBoundaries` This was unnecessary, since this method doesn't need to use `_rowCount`. Instead, make the asserts use only `idx` which will be maintained consistent with the waiter logic in this cache. �Ensure we only ever use `_rowCount` from the caching thread, so write reordering won't matter. * Don't include the SDK in our helix payload (#6918) * Don't include the SDK in our helix payload I noticed that the tests included the latest SDK - including the host - in our helix payloads. This is a large amount of unnecessary downloads and it also makes it so we use the latest host on the older frameworks which can fail when the latest host drops support for distros. Since our tests shouldn't need the full CLI, remove this from our helix payloads. We'll instead get just the runtime we need through `AdditionalDotNetPackage` * Place Helix downloaded runtime on the PATH Helix only sets the path when the CLI is included, however we don't need the CLI. * Make double assertions compare with tolerance instead of precision (#6923) Precision might cause small differences to round to a different number. Instead compare with a tolerance which is not sensitive to rounding. --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Michael Sharp <misharp@microsoft.com> Co-authored-by: Eric StJohn <ericstj@microsoft.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #6921
Asserting on
_rowCount < Utils.Size(_valueBoundaries)was catching a case where_rowCount's update was reordered before_valueBoundariesThis was unnecessary, since this method doesn't need to use
_rowCount.Instead, make the asserts use only
idxwhich will be maintained consistent with the waiter logic in this cache. This will lock and synchronize with the main thread to ensureidxis always within bounds.Ensure we only ever use
_rowCountfrom the caching thread, so write reordering won't matter.