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

Extend usage of GC_ALLOC_ALIGN8 #104781

Merged
merged 1 commit into from
Jul 13, 2024
Merged

Conversation

noahfalk
Copy link
Member

In the past we added FEATURE_64BIT_ALIGNMENT to support 8 byte alignment requirements on ARM, but for performance we also like to 8 byte align double arrays on other architectures. The GC has a GC_ALLOC_ALIGN8 flag that does that but it was only being used in the ARM case and we had a more complicated workaround being used elsewhere. This change expands GC_ALLOC_ALIGN8 so it is supported on all architectures and converges all our aligned double arrays to use that approach.

This change is also present in #100356 but extracting it out in a separate PR to make it easy to get it reviewed and merged.

@Maoni0 @jkotas @AaronRobinsonMSFT @davidwrighton

@noahfalk
Copy link
Member Author

fyi @chrisnas

Copy link
Contributor

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

@noahfalk
Copy link
Member Author

There is a CI failure in GC which looks like it may be caused by this change - I am investigating.

src/coreclr/gc/gcpriv.h Outdated Show resolved Hide resolved
@noahfalk
Copy link
Member Author

Just to capture it for the future, this is the GC assert seen prior to when I added RESPECT_LARGE_ALIGNMENT. No claim that it is causal, just that the assert was present prior to defining RESPECT_LARGE_ALIGNMENT and did not reoccur once RESPECT_LARGE_ALIGMENT was defined.

Console log: 'System.Text.Json.Tests' from job 42ba0bbf-f0ef-4f63-949a-afbdfdbee1e5 workitem 79ac3492-77e3-4ba8-bf03-035a305e4d33 (windows.10.amd64.open.rt) executed on machine a001XFN running Windows-10-10.0.14393-SP0
...

C:\h\w\C4520AF1\w\A820090F\e>"C:\h\w\C4520AF1\p\dotnet.exe" exec --runtimeconfig System.Text.Json.Tests.runtimeconfig.json --depsfile System.Text.Json.Tests.deps.json xunit.console.dll System.Text.Json.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Text.Json.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Text.Json.Tests (found 8609 of 8681 test cases)
  Starting:    System.Text.Json.Tests (parallel test collections = on [4 threads], stop on fail = off)

Assert failure(PID 9704 [0x000025e8], Thread: 1104 [0x0450]): new_current_total_committed_bookkeeping == current_total_committed_bookkeeping

CORECLR! WKS::gc_heap::verify_committed_bytes + 0x5A (0x71ac02aa)
CORECLR! WKS::gc_heap::plan_phase + 0x1F8B (0x71ab8c99)
CORECLR! WKS::gc_heap::gc1 + 0x155 (0x71aae971)
CORECLR! WKS::gc_heap::garbage_collect + 0x3E0 (0x71aae795)
CORECLR! WKS::GCHeap::GarbageCollectGeneration + 0x24C (0x71a9eb24)
CORECLR! WKS::gc_heap::trigger_gc_for_alloc + 0x40 (0x71abf5cc)
CORECLR! WKS::gc_heap::try_allocate_more_space + 0x12F (0x71abf7e4)
CORECLR! WKS::gc_heap::allocate_more_space + 0x14 (0x71aa3419)
CORECLR! WKS::GCHeap::Alloc + 0x185 (0x71a9dda5)
CORECLR! Alloc + 0x147 (0x7186ccda)
    File: D:\a\_work\1\s\src\coreclr\gc\gc.cpp:47449
    Image: C:\h\w\C4520AF1\p\dotnet.exe

@noahfalk
Copy link
Member Author

It appears that the bookkeeping assert is a previously known issue: #102706

In the past we added FEATURE_64BIT_ALIGNMENT to support 8 byte alignment
requirements on ARM, but for performance we also like to 8 byte align
double arrays on other architectures. The GC has a GC_ALLOC_ALIGN8 flag
that does that but it was only being used in the ARM case and we had
a more complicated workaround being used elsewhere. This change expands
GC_ALLOC_ALIGN8 so it is supported on all architectures and converges
all our aligned double arrays to use that approach.

GC_ALLOC_ALIGN8 only implies that the initial allocation is 8 byte
aligned, not that it will stay aligned after relocation. On ARM it will
stay aligned because RESPECT_LARGE_ALIGNMENT is defined but
on other 32 bit architectures it is not guaranteed to stay aligned.
@noahfalk noahfalk merged commit 46338d5 into dotnet:main Jul 13, 2024
89 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 12, 2024
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.

3 participants