Fix ThinSList size and unify EMPTY_BASES definitions#127911
Merged
Merged
Conversation
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/48302404-6028-4077-9e01-90f0d226f7ee Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/48302404-6028-4077-9e01-90f0d226f7ee Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/48302404-6028-4077-9e01-90f0d226f7ee Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/3a0b1409-2a17-4878-9e92-ae06a0676dfb Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/665e5e53-fc68-4f49-863b-33fbba8270b7 Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
max-charlamb
May 7, 2026 12:55
View session
max-charlamb
changed the title
EMPTY_BASES_DECL definitions; keep only the minipal oneEMPTY_BASES_DECL definitions
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes the EMPTY_BASES_DECL macro in minipal and applies it to SList so MSVC can perform empty base class optimization, reducing SList’s footprint (notably for thin/head-only lists) and aligning behavior across CoreCLR and NativeAOT.
Changes:
- Define
EMPTY_BASES_DECLinsrc/native/minipal/utils.h(MSVC:__declspec(empty_bases), otherwise empty). - Remove local/overriding
EMPTY_BASES_DECLdefinitions from CoreCLR PAL headers so the minipal definition is used consistently. - Apply
EMPTY_BASES_DECLtoSListand add debug-only layoutstatic_asserts to validate expected sizing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/native/minipal/utils.h | Adds the canonical EMPTY_BASES_DECL definition (MSVC vs non-MSVC). |
| src/coreclr/pal/inc/pal.h | Removes the unconditional empty EMPTY_BASES_DECL override so minipal’s definition can take effect. |
| src/coreclr/inc/slist.h | Includes minipal utils, applies EMPTY_BASES_DECL to SList, and adds layout validation assertions (currently _DEBUG-gated). |
| src/coreclr/inc/palclr.h | Removes the local EMPTY_BASES_DECL definition in favor of the minipal definition (palclr already includes <minipal/utils.h>). |
jkotas
reviewed
May 7, 2026
jkotas
approved these changes
May 7, 2026
max-charlamb
changed the title
EMPTY_BASES_DECL definitionsEMPTY_BASES definitions
Member
|
/ba-g test infra issue |
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
max-charlamb
approved these changes
May 10, 2026
Member
|
/ba-g test infra issue |
Open
3 tasks
jakobbotsch
pushed a commit
to jakobbotsch/runtime
that referenced
this pull request
May 12, 2026
Found that SList was taking up two pointer sizes because MSVC does not do empty base class optimization by default. We already have `EMPTY_BASES_DECL`, but it is not defined in NativeAOT. Given that SList is used in both CoreCLR and NativeAOT I also refactored `EMPTY_BASES_DECL` to be defined in minipal so it can be accessed by both builds. ## Changes - **`src/coreclr/pal/inc/pal.h`** — removed the unconditional `#define EMPTY_BASES_DECL` (always empty, wrong on MSVC). `pal.h` already `#include`s `<minipal/utils.h>` above that line, so the minipal definition takes effect automatically. - **`src/coreclr/inc/slist.h`** — switched to `#include <minipal/utils.h>` and `EMPTY_BASES_DECL` directly; moved `SListLayoutValidationElem` and related `static_assert`s under `#ifdef _DEBUG` so they don't affect release builds. `src/native/minipal/utils.h` is now the single source of truth for `EMPTY_BASES_DECL`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com> Co-authored-by: Max Charlamb <maxcharlamb@microsoft.com>
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
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.
Found that SList was taking up two pointer sizes because MSVC does not do empty base class optimization by default. We already have
EMPTY_BASES_DECL, but it is not defined in NativeAOT. Given that SList is used in both CoreCLR and NativeAOT I also refactoredEMPTY_BASES_DECLto be defined in minipal so it can be accessed by both builds.Changes
src/coreclr/pal/inc/pal.h— removed the unconditional#define EMPTY_BASES_DECL(always empty, wrong on MSVC).pal.halready#includes<minipal/utils.h>above that line, so the minipal definition takes effect automatically.src/coreclr/inc/slist.h— switched to#include <minipal/utils.h>andEMPTY_BASES_DECLdirectly; movedSListLayoutValidationElemand relatedstatic_asserts under#ifdef _DEBUGso they don't affect release builds.src/native/minipal/utils.his now the single source of truth forEMPTY_BASES_DECL.