-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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: retrieve .text coordinates for large pages #33116
src: retrieve .text coordinates for large pages #33116
Conversation
After locating the in-memory base address of the Node.js executable using `dl_iterate_phdr(3)` we open the on-disk executable file and extract the address and size of the `.text` section. We then add the in-memory base address to the offset to locate the `.text` section in memory. This approach removes the need for a symbol to tell us where the `.text` section is located, and, unlike the previous approach, guarantees that we will always have the entire `.text` section available for re-mapping. Signed-off-by: Gabriel Schulhof <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good idea for two reasons:
-
Won't work when
/proc
isn't mounted. -
Does a lot of extra file I/O at startup.
The second one in particular is a deal breaker.
src/large_pages/node_large_page.cc
Outdated
class_name(const class_name&) = delete; \ | ||
class_name(class_name&&) = delete; \ | ||
void operator= (const class_name&) = delete; \ | ||
void operator= (const class_name&&) = delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to have a DISALLOW_COPY_AND_ASSIGN
macro for this but we moved to just writing them out by hand, see #26634. Can you do that here too?
src/large_pages/node_large_page.cc
Outdated
struct stat file_info; | ||
if (stat(fname, &file_info) == -1) goto fail; | ||
|
||
fd_ = open(fname, O_RDONLY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open(2)
is interruptible by a signal. You should retry on EINTR:
fd_ = open(fname, O_RDONLY); | |
do | |
fd_ = open(fname, O_RDONLY); | |
while (fd == -1 && errno == EINTR); |
As well, stat + open is race-y. Not that it really matters here but you still should do open + fstat. :-)
FORCE_INLINE ~MappedFilePointer() { | ||
if (fd_ == -1) return; | ||
if (close(fd_) == 0) return; | ||
PrintSystemError(errno); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you if (errno == EINTR || errno == EINPROGRESS) return;
?
close(2)
is interruptible but you shouldn't retry - it's going to get closed anyway - and printing an error isn't useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this.
src/large_pages/node_large_page.cc
Outdated
for (uint32_t idx = 0; idx < ehdr->e_shnum; idx++) { | ||
const ElfW(Shdr)* sh = &shdrs[idx]; | ||
const char* section_name = static_cast<const char*>(&snames[sh->sh_name]); | ||
if (std::string(section_name) == ".text") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (std::string(section_name) == ".text") { | |
if (0 == strcmp(section_name, ".text")) { |
It's kind of wasteful to create a heap-allocated throwaway string. It's probably going to be stored inline for short strings but you get my point.
} | ||
ElfW(Shdr) text_section; | ||
#if defined(__linux__) | ||
dl_params.exename = "/proc/self/exe"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes /proc
is mounted but that need not be the case.
@bnoordhuis I checked the startup time wrt. master and it does not seem to be different at all with this change. This feature used to rely on reading node/src/large_pages/node_large_page.cc Lines 163 to 168 in f64c640
and node/src/large_pages/node_large_page.cc Lines 205 to 209 in f64c640
may be worth the renewed reliance on |
@bnoordhuis I also added a |
The kernel reads the ELF header on startup so it's often in the page cache, but when it's not and you're running node off your Raspberry Pi's SD card, you're going to notice.
procfs is a virtual fs though, there's no actual disk I/O taking place. |
@bnoordhuis fair enough. |
After locating the in-memory base address of the Node.js executable
using
dl_iterate_phdr(3)
we open the on-disk executable file andextract the address and size of the
.text
section. We then add thein-memory base address to the offset to locate the
.text
section inmemory.
This approach removes the need for a symbol to tell us where the
.text
section is located, and, unlike the previous approach,guarantees that we will always have the entire
.text
sectionavailable for re-mapping.
Signed-off-by: @gabrielschulhof
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes