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

pause in read of /proc/self/maps #708

Closed
therealkenc opened this issue Aug 1, 2016 · 10 comments
Closed

pause in read of /proc/self/maps #708

therealkenc opened this issue Aug 1, 2016 · 10 comments

Comments

@therealkenc
Copy link
Collaborator

therealkenc commented Aug 1, 2016

MS recently did a nice edge dev blog post on their progress on node-chakracore and I thought I'd see what WSL syscalls might be problematic, given that v8 works.

Out of the gate it fails on #597 because it tries to mmap 32GB (sic) of anonymous memory for reasons. Upping the NT paging file size does get you past that.

WSL then does admirably well with what's thrown at it, but makes very slow progress, because a read() syscall pauses when /proc/self/maps is queried. This an strace of npm install <anything>:

:58.099968 open("/proc/self/maps", O_RDONLY|O_CLOEXEC) = 14
:58.099968 getrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=8192*1024}) = 0
:58.100468 fstat(14, {st_mode=S_IFREG|0444, st_size=0, ...}) = 0
:58.100468 read(14, "00400000-01315000 r-x- 00000000 "..., 1024) = 1024   <--- BIG PAUSE
:59.121650 read(14, ".so.55.1\n7fff83516000-7fff835170"..., 1024) = 1024
:59.122150 read(14, ".23.so\n7fff83b15000-7fff83b19000"..., 1024) = 1024
:59.122150 read(14, "fff84050000-7fff841c7000 r-x- 00"..., 1024) = 1024
[...more reads in quick succession]
...

That first read after open at :58.100468 pauses for just over a second (1021ms) before proceeding. There are many hundreds (if not thousands) of such calls.

This is not worth looking into as a priority for obvious reasons, but maybe it tweaks a "I bet I know what's happening there" inspiration from someone. Installing the pre-release linked above is trivial.

@stehufntdev
Copy link
Collaborator

Thanks for the detailed write-up, we'll need to investigate this one. There was a bug filed internally on something similar where address space enumeration took a long time querying protection bits and it may be related.

@therealkenc
Copy link
Collaborator Author

@stehufntdev - Just a heads up also since you mentioned protection bits. On WSL the protection bits for /proc/self/maps aren't reported exactly the same as on native Ubuntu.

WSL:

$ cat /proc/self/maps
00400000-0040c000 r-x- 00000000 00:00 179095                     /bin/cat
[...]

Native:

$ cat /proc/self/maps
00400000-0040c000 r-xp 00000000 08:01 7340056                            /bin/cat
[...]

Note the missing p (private; copy on write) in r-xp. Unfortunately, a certain large popular body of software is very picky about that character being p, s, or S here, and fails. I assume this is hard to map to NT semantics, or you guys would have implemented it already. If so, it would probably more than suffice to just fake the bit to 'p' for the time being (if not forever).

@stehufntdev
Copy link
Collaborator

Thanks for the heads up, yes this is something we are tracking. If the chromium scenario or anything are interesting please give us feedback on the user voice page so we can prioritize.

@stehufntdev
Copy link
Collaborator

stehufntdev commented Apr 28, 2017

Most of the slowness was related to #776, and after that change the runtime of a Chakra test, test/Basics, dropped from 53 seconds to 20 seconds on my test vm. To get the runtime closer to native we'll need to look at targeted fixes for caching information for /proc/self/maps.

@therealkenc
Copy link
Collaborator Author

therealkenc commented Apr 28, 2017

Awesome. Thanks for the update -- I wasn't really following #776. I assume the remaining slowness is related to #1671.

Can I talk you into spending a few minutes and changing that '-' to a 'p' in the source for the next roll? It will be wrong for shared memory and right for private. Being right most of the time is better than being wrong all of the time. And it will unblock some stuff that just wants the string to be well formed.

@benhillis
Copy link
Member

@therealkenc - Stephen's fix addresses #1671 as well.

@stehufntdev
Copy link
Collaborator

Thanks for the reminder Ken, I'll take a look at adding the 'p'. When the procfs support was added we didn't have all the plumbing required but I'm pretty sure we have it all now. Marking this as a bug for tracking.

@stehufntdev
Copy link
Collaborator

We submitted a fix for converting the '-' to a 'p' or 's', and it should be out in insider builds soon.

@therealkenc
Copy link
Collaborator Author

@stehufntdev I just noticed the access permissions showed up in 16215. Thanks for getting this in. Given we're deeming the performance issue addressed I think this one can be marked fixedinsiderbuilds and closed. There are other issues open that can cover further "allocate all the things" perf improvements.

@stehufntdev
Copy link
Collaborator

Great, thanks for following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants