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

src: large page support for macOS #28977

Closed
wants to merge 3 commits into from

Conversation

@devnexen
Copy link
Contributor

commented Aug 5, 2019

Proposal to bring the support for this platform.
We assume the pse36 cpu flag is present for 2MB large page support present
in recent years in mac line (not to be backported to 10.x anyway).
Recommended better for mac production servers rather than casual mac books.

@devnexen devnexen force-pushed the devnexen:osx_large_page_support branch from a10814e to 6708496 Aug 5, 2019

@bnoordhuis
Copy link
Member

left a comment

Thanks, LGTM % some style nits.

if options.shared or options.enable_static:
raise Exception(
'Large pages are supported only while creating node executable.')
if target_arch!="x64":
raise Exception(
'Large pages are supported only x64 platform.')
if flavor == 'mac':
print("macOS server with 32GB or more is recommended")

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 6, 2019

Member

Style: single quotes

Substance: info('macOS server...') might be better although plain print() isn't exactly wrong either, it's used in plenty of places.

This comment has been minimized.

Copy link
@Trott

Trott Aug 19, 2019

Member

Putting @bnoordhuis's comment above as a suggestion for ease of adoption:

Suggested change
print("macOS server with 32GB or more is recommended")
info('macOS server with 32GB or more is recommended')

while (true) {
if (vm_region_recurse_64(mach_task_self(), &addr, &size, &depth,
(vm_region_info_64_t)&map, &count) != KERN_SUCCESS) {

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 6, 2019

Member

reinterpret_cast please, no C style casts (and preferably line up the arguments.)

tmem = mmap(start, size,
PROT_READ | PROT_WRITE | PROT_EXEC,
MAP_PRIVATE | MAP_ANONYMOUS,
VM_FLAGS_SUPERPAGE_SIZE_2MB, 0);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 6, 2019

Member

Can you line up the arguments with the ones on the first line?

src: large page support for macOS
Proposal to bring the support for this platform.
We assume the pse36 cpu flag is present for 2MB
large page support present in recent years
in mac line (not to be backported to 10.x anyway).
Recommended better for mac production servers rather
than casual mac books.

@devnexen devnexen force-pushed the devnexen:osx_large_page_support branch from 6708496 to 7a698d4 Aug 6, 2019

@nodejs-github-bot

This comment has been minimized.

@jasnell
jasnell approved these changes Aug 7, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

One remaining style nit?

@Trott Trott requested review from jasnell and bnoordhuis Aug 19, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Significant chunk of C++ code added, I think, since previous reviews, so re-requested reviews.

@nodejs-github-bot

This comment has been minimized.

@@ -382,9 +382,25 @@ MoveTextRegionToLargePages(const text_region& r) {
munmap(nmem, size);
return -1;
}
memcpy(tmem, nmem, size);
ret = mprotect(start, size, PROT_READ | PROT_WRITE | PROT_EXEC);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 20, 2019

Member

Can you explain why this is necessary? It's already mapped rwx, isn't it?

Should the mmap() call include MAP_FIXED in its flags like it does on Linux and FreeBSD? If not, can you update the comment atop of this function?

Aside: since there are a lot of munmap(nmem, size) calls now, it'd be nice to DRY it:

nmem = mmap(...);
OnScopeLeave munmap_on_return([nmem, size] () {
  if (-1 == munmap(nmem, size)) PrintSystemError(errno);
});

I guess you lose the ability to return an error from MoveTextRegionToLargePages() on munmap() failure but that doesn't seem too bad, it's not really a fatal error and probably hypothetical to boot. You could simply abort on failure.

This comment has been minimized.

Copy link
@devnexen

devnexen Aug 20, 2019

Author Contributor

To answer your first question it is rx but will add a comment (note that s start address not the new address). Ok for the rest (including the info change in configure.py).

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 20, 2019

Member

Just to round out this discussion: using MAP_FIXED is not an option on macos? start is properly aligned, isn't it?

This comment has been minimized.

Copy link
@devnexen

devnexen Aug 20, 2019

Author Contributor

start is indeed aligned like other platforms, MAP_FIXED failed on this platform though with 24MB close to the start address reservation attempt.

This comment has been minimized.

Copy link
@jasnell

jasnell Aug 20, 2019

Member

Yeah, I'd have to go digging for the reason again but MAP_FIXED has long had problems with mmap for large page sizes on macos. Pretty sure the macos api docs recommend not to use it but if I recall correctly they don't really explain why.

src/large_pages/node_large_page.cc Show resolved Hide resolved
tmem = mmap(start, size,
PROT_READ | PROT_WRITE | PROT_EXEC,
MAP_PRIVATE | MAP_ANONYMOUS,
VM_FLAGS_SUPERPAGE_SIZE_2MB, 0);
if (tmem == MAP_FAILED) {
PrintSystemError(errno);
munmap(nmem, size);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 20, 2019

Member

There are a few more munmap(nmem, size) calls in linux/freebsd code paths. Can you remove those as well?

@devnexen devnexen force-pushed the devnexen:osx_large_page_support branch from 4ac3692 to 0f7190f Aug 20, 2019

@@ -382,9 +382,25 @@ MoveTextRegionToLargePages(const text_region& r) {
munmap(nmem, size);
return -1;
}
memcpy(tmem, nmem, size);
ret = mprotect(start, size, PROT_READ | PROT_WRITE | PROT_EXEC);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 20, 2019

Member

Just to round out this discussion: using MAP_FIXED is not an option on macos? start is properly aligned, isn't it?

return IsSuperPagesEnabled();
#elif defined(__APPLE__)
// pse-36 flag is present in recent mac x64 products.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 20, 2019

Member

How recent is recent? Node.js supports macOS 10.11 and up.

This comment has been minimized.

Copy link
@devnexen

devnexen Aug 20, 2019

Author Contributor

Then it is fine El Capitan is recent enough :)

@nodejs-github-bot

This comment has been minimized.

Trott added a commit that referenced this pull request Aug 20, 2019
src: add large page support for macOS
Proposal to bring the support for this platform.
We assume the pse36 cpu flag is present for 2MB
large page support present in recent years
in mac line (not to be backported to 10.x anyway).
Recommended better for mac production servers rather
than casual mac books.

PR-URL: #28977
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Landed in 32df017.

Added dont-land-on-v10.x and dont-land-on-v8.x based on the commit message.

@Trott Trott closed this Aug 20, 2019

ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2019
src: add large page support for macOS
Proposal to bring the support for this platform.
We assume the pse36 cpu flag is present for 2MB
large page support present in recent years
in mac line (not to be backported to 10.x anyway).
Recommended better for mac production servers rather
than casual mac books.

PR-URL: nodejs#28977
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 3, 2019
src: add large page support for macOS
Proposal to bring the support for this platform.
We assume the pse36 cpu flag is present for 2MB
large page support present in recent years
in mac line (not to be backported to 10.x anyway).
Recommended better for mac production servers rather
than casual mac books.

PR-URL: #28977
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: add large page support for macOS
Proposal to bring the support for this platform.
We assume the pse36 cpu flag is present for 2MB
large page support present in recent years
in mac line (not to be backported to 10.x anyway).
Recommended better for mac production servers rather
than casual mac books.

PR-URL: nodejs#28977
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: add large page support for macOS
Proposal to bring the support for this platform.
We assume the pse36 cpu flag is present for 2MB
large page support present in recent years
in mac line (not to be backported to 10.x anyway).
Recommended better for mac production servers rather
than casual mac books.

PR-URL: nodejs#28977
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.