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.

Sign up for GitHub

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

ZOS Use Default Large Page Type Within Objectheap and Codecache #7585

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ZOS Use Default Large Page Type Within Objectheap and Codecache #7585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
50 changes: 16 additions & 34 deletions runtime/compiler/control/J9Options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1454,10 +1454,8 @@ J9::Options::fePreProcess(void * base)

UDATA pageSizeHowMany = 0;
UDATA pageSizeOptionNumber = 0;
UDATA pageableHowMany = 0;
UDATA pageableOptionNumber = 0;
UDATA nonPageableHowMany = 0;
UDATA nonPageableOptionNumber = 0;
UDATA pageableOptionNumber = 0;

char *optionsString = NULL;

Expand All @@ -1467,15 +1465,15 @@ J9::Options::fePreProcess(void * base)
// optionsString can not be NULL here, though it may point to null ('\0') character
char *scanLimit = optionsString + strlen(optionsString);

// Parse -Xlp:codecache:pagesize=<size> option.
// The proper formed -Xlp:codecache: options include
// For X and zLinux platforms:
// -Xlp:codecache:pagesize=<size> or
// -Xlp:codecache:pagesize=<size>,pageable or
// -Xlp:codecache:pagesize=<size>,nonpageable
// For zOS platforms
// -Xlp:codecache:pagesize=<size>,pageable or
// -Xlp:codecache:pagesize=<size>,nonpageable
/* Parse -Xlp:codecache:pagesize=<size> option.
* The proper formed -Xlp:codecache: options include
* For All platforms:
* -Xlp:codecache:pagesize=<size> or
* -Xlp:codecache:pagesize=<size>,pageable or
* -Xlp:codecache:pagesize=<size>,nonpageable
*
* Nonpageable codecache does not support large pages on ZOS.
*/
while (optionsString < scanLimit)
{
if (try_scan(&optionsString, ","))
Expand Down Expand Up @@ -1602,16 +1600,12 @@ J9::Options::fePreProcess(void * base)
}
else if (try_scan(&optionsString, "pageable"))
{
pageableHowMany += 1;
pageableOptionNumber = optionNumber;

parsingState = XLPCC_PARSING_COMMA;
}
else if (try_scan(&optionsString, "nonpageable"))
{
nonPageableHowMany += 1;
nonPageableOptionNumber = optionNumber;

parsingState = XLPCC_PARSING_COMMA;
}
}
Expand Down Expand Up @@ -1642,25 +1636,13 @@ J9::Options::fePreProcess(void * base)
return false;
}

#if defined(J9ZOS390)
// [non]pageable option must be specified for Z platforms
if ((0 == pageableHowMany) && (0 == nonPageableHowMany))
{
// [non]pageable not found
char *xlpOptionErrorString = "-Xlp:codecache:";
char *xlpMissingOptionString = "[non]pageable";

j9nls_printf(PORTLIB, J9NLS_ERROR, J9NLS_JIT_OPTIONS_XLP_INCOMPLETE_OPTION, xlpOptionErrorString, xlpMissingOptionString);
return false;
}

// Check for the right most option is most right
if (pageableOptionNumber > nonPageableOptionNumber)
requestedLargeCodePageFlags = J9PORT_VMEM_PAGE_FLAG_PAGEABLE;
else
#if defined(J9ZOS390)
/* Only set for the right most option, ignoring previous instances */
if (nonPageableOptionNumber > pageableOptionNumber)
requestedLargeCodePageFlags = J9PORT_VMEM_PAGE_FLAG_FIXED;

#endif // defined(J9ZOS390)
else
requestedLargeCodePageFlags = J9PORT_VMEM_PAGE_FLAG_PAGEABLE;
#endif /* defined(J9ZOS390) */

// print extra comma ignored warning
if (extraCommaWarning)
Expand Down
47 changes: 22 additions & 25 deletions runtime/gc_modron_startup/mmparse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,9 @@ xlpSubOptionsParser(J9JavaVM *vm, IDATA xlpIndex, XlpError *xlpError, UDATA *req

UDATA pageSizeHowMany = 0;
#if defined(J9ZOS390)
UDATA pageableHowMany = 0;
UDATA pageableOptionNumber = 0;
UDATA nonPageableHowMany = 0;
UDATA nonPageableOptionNumber = 0;
UDATA pageableOptionNumber = 0;
PORT_ACCESS_FROM_JAVAVM(vm);
#endif /* defined(J9ZOS390) */

/* Reset error state from parsing of previous -Xlp<size> option */
Expand Down Expand Up @@ -482,13 +481,11 @@ xlpSubOptionsParser(J9JavaVM *vm, IDATA xlpIndex, XlpError *xlpError, UDATA *req
parsingState = PARSING_COMMA;
} else if (try_scan(&optionsString, "pageable")) {
#if defined(J9ZOS390)
pageableHowMany += 1;
pageableOptionNumber = optionNumber;
#endif /* defined(J9ZOS390) */
parsingState = PARSING_COMMA;
} else if (try_scan(&optionsString, "nonpageable")) {
#if defined(J9ZOS390)
nonPageableHowMany += 1;
nonPageableOptionNumber = optionNumber;
#endif /* defined(J9ZOS390) */
parsingState = PARSING_COMMA;
Expand Down Expand Up @@ -535,24 +532,28 @@ xlpSubOptionsParser(J9JavaVM *vm, IDATA xlpIndex, XlpError *xlpError, UDATA *req
}

#if defined(J9ZOS390)
/*
* [non]pageable
* - this option must be specified for Z platforms
*/
if ((0 == pageableHowMany) && (0 == nonPageableHowMany)) {
/* error: [non]pageable not found */
xlpErrorState = XLP_INCOMPLETE_OPTION;
xlpError->xlpOptionErrorString = "-Xlp:objectheap:";
xlpError->xlpMissingOptionString = "[non]pageable";
return xlpErrorState;
}

if (pageableOptionNumber > nonPageableOptionNumber) {
if (nonPageableOptionNumber > pageableOptionNumber) {
/* nonpageable is most right */
*requestedPageFlags = J9PORT_VMEM_PAGE_FLAG_FIXED;
} else if (pageableOptionNumber != 0) {
/* pageable is most right */
*requestedPageFlags = J9PORT_VMEM_PAGE_FLAG_PAGEABLE;
} else {
/* nonpageable is most right */
*requestedPageFlags = J9PORT_VMEM_PAGE_FLAG_FIXED;
/* [non]pageable not passed. By default use pageable if both are sizes are available */
UDATA *pageSizes = j9vmem_supported_page_sizes();
UDATA *pageFlags = j9vmem_supported_page_flags();
for (UDATA pageIndex = 0; 0 != pageSizes[pageIndex]; ++pageIndex) {
if (pageSizes[pageIndex] == (UDATA)requestedPageSize) {

if (pageFlags[pageIndex] == J9PORT_VMEM_PAGE_FLAG_PAGEABLE) {
*requestedPageFlags = J9PORT_VMEM_PAGE_FLAG_PAGEABLE;
break;
}
else {
*requestedPageFlags = J9PORT_VMEM_PAGE_FLAG_FIXED;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks in wrong place. Parser does context free check. This code uses context (pages available on OS).
Looks like it should be somewhere in j9vmem_find_valid_page_size()

Copy link
Contributor Author

@AlenBadel AlenBadel Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So directly implemented into
https://github.com/eclipse/omr/blob/8b719f55ba3d8b8051b40b6dc4b38a1caa9afe5e/port/zos390/omrvmem.c#L1331

Copy link
Contributor

@dmitripivkine dmitripivkine Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So directly implemented into
https://github.com/eclipse/omr/blob/8b719f55ba3d8b8051b40b6dc4b38a1caa9afe5e/port/zos390/omrvmem.c#L1331

I believe this is the question.
I think you should take a look. It supports OMRPORT_VMEM_PAGE_FLAG_NOT_USED so there is a mechanism to handle unspecified [non]pageable there. However it seems does it differently and favour FIXED in this case as a legacy support.
I believe we need a discussion between ZOS - involved specialists about this proposed behaviour change (including motivation)

Copy link
Contributor Author

@AlenBadel AlenBadel Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I believe we need a discussion between ZOS - involved specialists about this proposed behaviour change (including motivation)

@joransiu Has already expressed that 1M pagerable large pages are preferred. If we don't have any obvious drawbacks I don't see why this can't be default behaviour. I think it adds value that the the command line option can be symmetrical along all architectures.

It's obvious on the codecache side that we don't have a choice of using nonpageable in the first place and mandatory requiring an additional option doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make this change once we're on the same page and I'll let others chime in.

}
#endif /* defined(J9ZOS390) */

Expand Down Expand Up @@ -640,14 +641,10 @@ gcParseXlpOption(J9JavaVM *vm)
* It also overrides -Xlp<size> option if it appears to the right of -Xlp<size>
*
* The proper formed -Xlp:objectheap: option must be (in strict order):
* For all non-Z platforms:
* For all platforms:
* -Xlp:objectheap:pagesize=<size> or
* -Xlp:objectheap:pagesize=<size>,pageable or
* -Xlp:objectheap:pagesize=<size>,nonpageable
*
* For Z platforms
* -Xlp:objectheap:pagesize=<size>,pageable or
* -Xlp:objectheap:pagesize=<size>,nonpageable
*/
xlpObjectHeapIndex = FIND_AND_CONSUME_ARG(STARTSWITH_MATCH, "-Xlp:objectheap:", NULL);

Expand Down