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

Fix BigInteger outerloop test #111841

Merged
merged 4 commits into from
Jan 30, 2025
Merged

Conversation

Rob-Hague
Copy link
Contributor

This has presumably been failing since capping BigInteger to a maximum length

fixes #111814

This has presumably been failing since capping BigInteger to a maximum length
@tannergooding
Copy link
Member

/azp run runtime-libraries-coreclr outerloop-linux

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member

/azp run runtime-libraries-coreclr outerloop-linux

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member

/azp run runtime-libraries-coreclr outerloop-linux

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rob-Hague
Copy link
Contributor Author

😕

@tannergooding
Copy link
Member

/azp run runtime-libraries-coreclr outerloop-windows

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member

Seems to be specific to the Linux containers. Let me ask a bit internally on this.

@ericstj
Copy link
Member

ericstj commented Jan 29, 2025

Could just be they have a less memory, or a combination of less memory + concurrency. Could also be this test contributing

[OuterLoop("Takes a long time, allocates a lot of memory")]
[SkipOnMono("Frequently throws OOM on Mono")]
public static void ToString_ValidLargeFormat()

Here's all the Outerloop tests to consider https://github.com/search?q=repo%3Adotnet%2Fruntime%20path%3Asrc%2Flibraries%2FSystem.Runtime.Numerics%2Ftests%20Outerloop&type=code

@tannergooding
Copy link
Member

Could also be this test contributing

Ah, good point. I didn't think about other outerloop tests in the same project.

That test in particular should probably use TryFormat instead, in which case we should get false because the buffer was too small in contrast to FormatException which indicates the format is bad (too large). That would also allow it to run in the innerloop, as it would take nanoseconds instead.

The other outerloop don't look to be doing anything "significant" memory wise, just doing extended validation that isn't required to run every PR.

@tannergooding
Copy link
Member

@Rob-Hague is that something you'd be willing to try and fix as part of this PR? If not, I can push a commit up and try to finish getting CI passing for what's left

The change you have provided already is much appreciated and looks like it should help the original issue.

@Rob-Hague
Copy link
Contributor Author

Took a quick look, even when using TryFormat(Span<char>.Empty, ... ) we still end up down this path with nMaxDigits ~= 1e9 for the exponential format:

while (--nMaxDigits > 0)
{
vlb.Append(TChar.CastFrom((*dig != 0) ? (char)(*dig++) : '0'));
}

The 'G' format has an early out so does not suffer the same problem. I have just commented the exponential tests since I have not determined whether an early out would always be valid.

I am signing off for the day, feel free to change or revert things in this branch

@tannergooding
Copy link
Member

/azp run runtime-libraries-coreclr outerloop-linux

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member

Tests are passing now. There's some other unrelated failures still, but those are independent of what's being fixed here.

@tannergooding tannergooding merged commit 56b7d0e into dotnet:main Jan 30, 2025
86 of 89 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Jan 30, 2025
* main: (31 commits)
  More native AOT Pri-1 test tree bring up (dotnet#111994)
  Fix BigInteger outerloop test (dotnet#111841)
  JIT: Run 3-opt once across all regions (dotnet#111989)
  JIT: Check for profile consistency throughout JIT backend (dotnet#111684)
  [JIT] Add legacy extended EVEX encoding and EVEX.ND/NF feature to x64 emitter backend (dotnet#108796)
  [iOS][globalization] Fix IndexOf on empty strings on iOS to return -1 (dotnet#111898)
  System.Speech: Use intellisense xml from dotnet-api-docs (dotnet#111983)
  [mono][mini] Disable inlining if we encounter class initialization failure (dotnet#111754)
  [main] Update dependencies from dotnet/roslyn (dotnet#111946)
  Update dependencies from https://github.com/dotnet/arcade build 20250129.2 (dotnet#111996)
  Try changing the ICustomQueryInterface implementation to always return NotHandled instead of Failed to defer back to the ComWrappers impl. (dotnet#111978)
  Combined dependency update (dotnet#111852)
  Replace OPTIMIZE_FOR_SIZE with feature switch (dotnet#111743)
  Fix failed assertion 'FPbased == FPbased2' (dotnet#111787)
  Add remark to `ConditionalSelect` (dotnet#111945)
  JIT: fix try region cloning when try is nested in a handler (dotnet#111975)
  Use IRootFunctions in Tensor.StdDev (dotnet#110641)
  Remove zlib dependencies from Docker containers (dotnet#111939)
  Avoid `Unsafe.As` for `Memory<T>` and `ReadOnlyMemory<T>` conversion (dotnet#111023)
  Cleanup membarrier portability (dotnet#111943)
  ...
@Rob-Hague Rob-Hague deleted the bigintouterloop branch January 31, 2025 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[outerloop] Overflow in osx arm64 in System.Numerics.Tests.GetBitLengthTests.RunGetBitLengthTestsLarge
3 participants