698588 - Optimize DecommitFreePages

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

DecommitFreePages is in a slow path, so it was not built with speed in mind.  Currently we make one syscall per page when decommitting.  We could, relatively easily, make a single syscall to decommit multiple adjacent pages.

Comment 1

[Varun Vijayaraghavan]

Hello, I would be interested in giving this a try. I am quite comfortable with C++, but I have not worked with garbage collection. Do let me know if you would be interested in taking me through this. Thanks!

Comment 2

[Terrence Cole [:terrence]]

Awesome!  I'd be glad to have some help with this.  The first step is to follow along with https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey to get a working build of the shell and to run a benchmark against it to get a feel for what sort of numbers are normal on your machine.

You should also join us on IRC (https://wiki.mozilla.org/IRC).  The spidermonkey hackers hang out in the #jsapi channel and asking questions there is the best way to get through problems.  When you have a working build and are ready to move forward, send me a ping there (e.g. send "terrence: ping" on #jsapi when I am online and not away) and we can figure out where to go next.

Comment 3

[Terrence Cole [:terrence]]

This bug should address two problems.

1) Right now {Commit|Decommit}Memory in jsgcchunk.{h|cpp} are not clearly indicated as infallable (they return bool).  This is important because there is a stronger, fallable decommit operation that we are _not_ performing here.  The stronger version would not work with what we are doing next, so we need to fix this API before we do that.  Read the comments in bug 670596 to get the full background on our different deallocation options and why we are doing things the way they are done right now.

2) Right now DecommitFreePages calls DecommitMemory one page at a time (note: 1 page == 1 arena in the JS allocator).  If we have multiple adjacent arenas that we want to decommit, we could pass all of them to DecommitMemory at once, saving us a bunch of syscalls.  You would need to modify DecommitFreePages to coalesce decommit of adjacent pages into a single call.

The first modification is trivial, so both of these modifications can go in a single patch.

Varun, what is your status?  Have you gotten a working SpiderMonkey shell build and run the test suites?  If so, doing the first of these should be as simple as modifying CommitMemory and DecommitMemory and making sure the shell still builds and passes its tests.

Comment 4

[Varun Vijayaraghavan]

I have got a working shell build, and I have run the test suites. I will read up on the bug as soon as I can.

Comment 5

[Terrence Cole [:terrence]]

On IRC, bsdunx pointed out that VirtualAlloc calls VirtualAllocEx internally.  We could save a jump by calling VirtualAllocEx directly with the default parameter.  This is unlikely to have a significant performance effect, especially if we reduce the number of calls, but is something to keep in mind, since it is so trivial to implement.

Comment 6

[Terrence Cole [:terrence]]

On IRC, timeless pointed out that VirtualAllocEx's hProcess parameter is marked as __in and not __in_opt, so this conversion would not be trivial, and would further require more system calls.  This is definitely not worth doing.

Comment 7

[Igor Bukanov]

(In reply to Terrence Cole [:terrence] from comment #5)
> On IRC, bsdunx pointed out that VirtualAlloc calls VirtualAllocEx
> internally.  We could save a jump by calling VirtualAllocEx directly with
> the default parameter.

Do we know this for each and every version of Windows? Do we know that MS would not add an optimized version of VirtuallAlloc after they get reports about a potential slowdown in it? 

And even if the answer to both of these questions is true, the extra function call does not matter when it wraps an expensive system call. Given that I would prefer to save on code size even if we are talking about an extra NULL argument and save on comments. Those comments would explain otherwise why we think that we are smarter than MS docs that does not suggest to use the Ex version in place of plain VirtuallAlloc.

Comment 8

[Terrence Cole [:terrence]]

Decommit is now done in the background with 702251.