Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[Arm64/Unix] WIP-DoNotMerge-Draft 64K static support #10891

Closed
wants to merge 2 commits into from
+16 −14
Closed

[Arm64/Unix] WIP-DoNotMerge-Draft 64K static support #10891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
864f227
Draft 64K static support
sdmaclea Apr 10, 2017
3d7aec5
Use 64K page for GC
sdmaclea Apr 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/ToolBox/SOS/Strike/gcroot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1317,9 +1317,9 @@ void PrintNotReachableInRange(TADDR rngStart, TADDR rngEnd, BOOL bExcludeReadyFo
// In the code we also rely on the assumption that one card_table entry (DWORD) covers an entire os page
//
#if defined (_TARGET_WIN64_)
#define card_size ((size_t)(2*DT_OS_PAGE_SIZE/card_word_width))
#define card_size ((size_t)(2*4096/card_word_width))
#else
#define card_size ((size_t)(DT_OS_PAGE_SIZE/card_word_width))
#define card_size ((size_t)(4096/card_word_width))
#endif //_TARGET_WIN64_
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to stop duplicating this GC code here.


// so card_size = 128 on x86, 256 on x64
Expand Down
2 changes: 1 addition & 1 deletion src/ToolBox/SOS/Strike/platformspecific.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,6 @@ struct DT_RTL_USER_PROCESS_PARAMETERS

#endif // !FEATURE_PAL

#define DT_OS_PAGE_SIZE 4096
#define DT_OS_PAGE_SIZE 0x10000

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be complications here for remote debugging with target having separate size as host.

Also this may need a different mechanism to determine the os page size dynamically.

#endif // !__PLATFORM_SPECIFIC_INCLUDED
2 changes: 1 addition & 1 deletion src/gc/env/gcenv.base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ typedef DPTR(uint8_t) PTR_uint8_t;

#define DECLSPEC_ALIGN(x) __declspec(align(x))

#define OS_PAGE_SIZE 4096
#define OS_PAGE_SIZE 0x10000

#ifndef _ASSERTE
#define _ASSERTE(_expr) ASSERT(_expr)
Expand Down
8 changes: 4 additions & 4 deletions src/gc/gc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6306,7 +6306,7 @@ void gc_heap::make_c_mark_list (uint8_t** arr)
static const size_t card_bundle_word_width = 32;

// How do we express the fact that 32 bits (card_word_width) is one uint32_t?
static const size_t card_bundle_size = (size_t)(OS_PAGE_SIZE / (sizeof(uint32_t)*card_bundle_word_width));
static const size_t card_bundle_size = (size_t)(4096 / (sizeof(uint32_t)*card_bundle_word_width));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a name for this magic number. Suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GC_PAGE_SIZE


inline
size_t card_bundle_word (size_t cardb)
Expand Down Expand Up @@ -18786,13 +18786,13 @@ void gc_heap::fix_card_table ()
time_stop - time_start, tot_cycles);
#endif //TIME_WRITE_WATCH

assert( ((card_size * card_word_width)&(OS_PAGE_SIZE-1))==0 );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows, this really is the true operating system page size and not an arbitrary constant. The array of pointers that we receive from GetWriteWatch (used to track writes on the heap during concurrent GCs) are page-sized. When we're looping over this array, we're looking at single OS pages and setting cards for it.

I don't think that GetWriteWatch supports anything other than 4k pages, so it's probably fine to replace these uses with 4096, but perhaps we should assert somewhere that if we are using GetWriteWatch, OS_PAGE_SIZE is what we expect.

assert( ((card_size * card_word_width)&(4096-1))==0 );
//printf ("%Ix written into\n", bcount);
dprintf (3,("Found %Id pages written", bcount));
for (unsigned i = 0; i < bcount; i++)
{
// Set the card words corresponding to the entire page.
for (unsigned j = 0; j < (card_size*card_word_width)/OS_PAGE_SIZE; j++)
for (unsigned j = 0; j < (card_size*card_word_width)/4096; j++)
{
card_table [card_word (card_of (g_addresses [i]))+j] = ~0u;
}
Expand Down Expand Up @@ -26610,7 +26610,7 @@ void gc_heap::revisit_written_pages (BOOL concurrent_p, BOOL reset_only_p)
card_table [card_word (card_of (background_written_addresses [i]))] = ~0u;
dprintf (3,("Set Cards [%p:%p, %p:%p[",
card_of (background_written_addresses [i]), g_addresses [i],
card_of (background_written_addresses [i]+OS_PAGE_SIZE), background_written_addresses [i]+OS_PAGE_SIZE));
card_of (background_written_addresses [i]+4096), background_written_addresses [i]+4096));
#endif //NO_WRITE_BARRIER
uint8_t* page = (uint8_t*)background_written_addresses[i];
dprintf (3, ("looking at page %d at %Ix(h: %Ix)", i,
Expand Down
4 changes: 2 additions & 2 deletions src/gc/gcpriv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4311,9 +4311,9 @@ dynamic_data* gc_heap::dynamic_data_of (int gen_number)
// In the code we also rely on the assumption that one card_table entry (uint32_t) covers an entire os page
//
#if defined (BIT64)
#define card_size ((size_t)(2*OS_PAGE_SIZE/card_word_width))
#define card_size ((size_t)(8192/card_word_width))
#else
#define card_size ((size_t)(OS_PAGE_SIZE/card_word_width))
#define card_size ((size_t)(4096/card_word_width))
#endif // BIT64

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These may be better converted to hard numbers. Since they are supposedly empirical.

#if defined (_TARGET_WIN64_)
#define card_size 0x200
#else
#define card_size 0x100
#endif

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are empirical, yes, but they are also tied to GetWriteWatch operating at page granularity. If we change these, we'll also need to change the software write watch tables and card bundle tables (cc @adityamandaleeka)

// Returns the index of the card word a card is in
Expand Down
2 changes: 1 addition & 1 deletion src/inc/switches.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
#define USE_UPPER_ADDRESS 0

#elif defined(_TARGET_AMD64_) || defined(_TARGET_ARM64_)
#define PAGE_SIZE 0x1000
#define PAGE_SIZE 0x10000
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Page size will become dynamic

#define UPPER_ADDRESS_MAPPING_FACTOR 2
#define CLR_UPPER_ADDRESS_MIN 0x64400000000
#define CODEHEAP_START_ADDRESS 0x64480000000
Expand Down
2 changes: 1 addition & 1 deletion src/pal/src/include/pal/virtual.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ enum VIRTUAL_CONSTANTS
VIRTUAL_EXECUTE,
VIRTUAL_EXECUTE_READ,

VIRTUAL_PAGE_SIZE = 0x1000,
VIRTUAL_PAGE_SIZE = 0x10000,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Virtual page size will become dynamic an so will VIRTUAL_PAGE_SIZE_MASK

VIRTUAL_PAGE_MASK = VIRTUAL_PAGE_SIZE - 1,
BOUNDARY_64K = 0xffff
};
Expand Down
2 changes: 2 additions & 0 deletions src/utilcode/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,8 @@ LPVOID ClrVirtualAllocAligned(LPVOID lpAddress, SIZE_T dwSize, DWORD flAllocatio

#else // !FEATURE_PAL

if(alignment < PAGE_SIZE) alignment = PAGE_SIZE;

// UNIXTODO: Add a specialized function to PAL so that we don't have to waste memory
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

64K page size will increase the amount of wasted memory

dwSize += alignment;
SIZE_T addr = (SIZE_T)ClrVirtualAlloc(lpAddress, dwSize, flAllocationType, flProtect);
Expand Down
2 changes: 1 addition & 1 deletion src/zap/zapimage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,7 +1547,7 @@ void ZapImage::OutputTables()

#if defined(FEATURE_PAL)
// PAL library requires native image sections to align to page bounaries.
SetFileAlignment(0x1000);
SetFileAlignment(0x10000);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section alignment to match maximum supported page size.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this will make native images a lot bigger. I am not sure whether this would be acceptable.

How are the native binary formats like ELF dealing with the 64k pages? Do they have the sections aligned at 64k boundaries as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas Based on what I read in golang/go#10180 (cited by @vielmetti). I think ELF will be aligning the sections to 64K page boundaries. However the ELF file size will not be padded to fill 64K, it will truncated as needed.

It is possible we could support file with 4K alignment, but PAL loader would need to be updated. This seems feasible since we have RELOC_PAGE_SIZE set to 4K.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdmaclea what do you mean by RELOC_PAGE_SIZE? I don't see such a symbol defined anywhere.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janvorli src/zap/zaprelocs.h:#define RELOCATION_PAGE_SIZE 0x1000

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas For the smallest files the size is growing about the same amount

 12288  4KAligned/System.Xml.XPath.ni.dll
196608 64KAligned/System.Xml.XPath.ni.dll

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas I'll see if I can fix the native image loader to not require 64K alignment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may to tweak the file format so that we can get the page alignment, without the padding.

Copy link
Author

@sdmaclea sdmaclea Apr 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are the native binary formats like ELF dealing with the 64k pages? Do they have the sections aligned at 64k boundaries as well?

@jkotas We objdumped an elf. The sections are only aligned to something like 8 byte boundaries. The VA of the section is incremented by a multiple of the maximum page size. I think for Arm64 is should be 16byte boundaries.

I took a look at "Visual Studio, Microsoft Portable Executable and Common Object File Format Specification Revision 9.3 – December 29, 2015" the following statement appears in the text.

If the SectionAlignment is less than the architecture’s page size, then 
FileAlignment must match SectionAlignment.

I think this would allow the PE to behave similarly to ELF.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I created PR #10959 with the ZapWriter Loader rework to prevent this bloat.

#elif defined(_TARGET_ARM_) && defined(FEATURE_CORESYSTEM)
if (!IsReadyToRunCompilation())
{
Expand Down
2 changes: 1 addition & 1 deletion src/zap/zapwriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void ZapWriter::Initialize()
m_FileAlignment = 0x200;
}

#define SECTION_ALIGNMENT 0x1000
#define SECTION_ALIGNMENT 0x10000

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section alignment to match maximum supported page size.

void ZapWriter::Save(IStream * pStream)
{
Expand Down