Report DOM partitionAlloc size to V8 GC.

Report DOM partitionAlloc size to V8 GC.

Background: Currently V8GC do not have idea of DOM memory usage retained by wrappers. A small set of unreachable wrappers may retain huge DOM, but v8 gc may not be triggered as it is not aware of DOM memory usage.

This CL introduces |V8GCController::reportDOMMemoryUsageToV8|
- DOM memory usage is tracked at partition{Alloc,Free}
- After each task run, |reportDOMMemoryUsageToV8| is called.
- |reportDOMMemoryUsageToV8| notifies V8 GC the partition memory size currently consumed by DOM.

After this CL, http://jsbin.com/nitobiru/18 will not crash.

BUG=365018, 368406
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176006

[kouhei (in TOK), 2014-05-28 06:56:54]

Would you take a look?

[haraken, 2014-05-28 07:11:33]

If performance permits, I think this is a good approach.

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:110:
Partitions::addDOMMemoryUsage(partitionAllocGetSize(ptr));

Won't this affect performance? I think this call path is very
performance-sensitive.

I think you can check performance using Dromaeo/dom-*.

[haraken, 2014-05-28 07:11:57]

+jochen, hpayer

[jochen (gone - plz use gerrit), 2014-05-28 07:23:02]

https://codereview.chromium.org/301743006/diff/1/Source/bindings/v8/V8GCContr...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/301743006/diff/1/Source/bindings/v8/V8GCContr...
Source/bindings/v8/V8GCController.cpp:424: if (!isMainThread())
why?

https://codereview.chromium.org/301743006/diff/1/Source/platform/Partitions.h
File Source/platform/Partitions.h (right):

https://codereview.chromium.org/301743006/diff/1/Source/platform/Partitions.h...
Source/platform/Partitions.h:60: static SizeSpecificPartitionAllocator<3072>
m_objectModelAllocator;
why not just report the size of this partition?

[kouhei (in TOK), 2014-05-28 07:35:20]

https://codereview.chromium.org/301743006/diff/1/Source/bindings/v8/V8GCContr...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/301743006/diff/1/Source/bindings/v8/V8GCContr...
Source/bindings/v8/V8GCController.cpp:424: if (!isMainThread())
On 2014/05/28 07:23:03, jochen wrote:
> why?

We only want to notify main thread isolate, as all DOM Node objects should be in
main thread.

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:110:
Partitions::addDOMMemoryUsage(partitionAllocGetSize(ptr));
On 2014/05/28 07:11:34, haraken wrote:
> 
> Won't this affect performance? I think this call path is very
> performance-sensitive.
> 
> I think you can check performance using Dromaeo/dom-*.
Ack. Will test.

https://codereview.chromium.org/301743006/diff/1/Source/platform/Partitions.h
File Source/platform/Partitions.h (right):

https://codereview.chromium.org/301743006/diff/1/Source/platform/Partitions.h...
Source/platform/Partitions.h:60: static SizeSpecificPartitionAllocator<3072>
m_objectModelAllocator;
On 2014/05/28 07:23:03, jochen wrote:
> why not just report the size of this partition?

Is there an easy way to do that?

[jochen (gone - plz use gerrit), 2014-05-28 07:55:23]

it would be nice if we could also report usage on workers.

we could report totalSizeOfSuperPages

[Jens Widell, 2014-05-28 08:04:41]

On 2014/05/28 07:55:23, jochen wrote:
> we could report totalSizeOfSuperPages

That has a 2 MB granularity, doesn't include allocations larger than 1 MB, and
never decreases (super pages are only freed at shutdown.)

[esprehn, 2014-05-28 08:38:13]

I'm not sure this is correct, you're telling v8 about all the nodes in Shadow
trees, the web inspector, lots of things that are totally unrelated to the
current page and that v8 may not even be able to reclaim at all.

If you're going to do this you might as well just tell v8 about the size of all
Partition alloc partitions.

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:110:
Partitions::addDOMMemoryUsage(partitionAllocGetSize(ptr));
Lots of nodes are not reachable or even reclaimable by v8.

[kouhei (in TOK), 2014-05-28 08:53:23]

Dromaeo regression is huge: (w/o patch) http://dromaeo.com/?id=222058 ->
(w/patch) http://dromaeo.com/?id=222057
DOM Modification: 562.39runs/s -> 512.63runs/s
OM Query: 27528.80rns/s -> 23163.84runs/s

On 2014/05/28 08:38:13, esprehn wrote:
> I'm not sure this is correct, you're telling v8 about all the nodes in Shadow
> trees, the web inspector, lots of things that are totally unrelated to the
> current page and that v8 may not even be able to reclaim at all.
Yes. This is true.

However, we currently do not have a way to know exactly how much DOM memory is
retained by v8 wrappers.

The original plan was to v8::LowMemoryNotification when partitionAlloc is using
too much mem.
As this would force major gc multiple times, I received concerns from v8 team.

The next plan was to implement an alternative api to tell this to v8. (
https://code.google.com/p/v8/issues/detail?id=3338&thanks=3338&ts=1400591978 )
Before that we wanted to know if it really worth adding a new api.

[haraken, 2014-05-28 09:06:14]

On 2014/05/28 08:53:23, kouhei wrote:
> Dromaeo regression is huge: (w/o patch) http://dromaeo.com/?id=222058 ->
> (w/patch) http://dromaeo.com/?id=222057
> DOM Modification: 562.39runs/s -> 512.63runs/s
> OM Query: 27528.80rns/s -> 23163.84runs/s
> 
> On 2014/05/28 08:38:13, esprehn wrote:
> > I'm not sure this is correct, you're telling v8 about all the nodes in
Shadow
> > trees, the web inspector, lots of things that are totally unrelated to the
> > current page and that v8 may not even be able to reclaim at all.
> Yes. This is true.
> 
> However, we currently do not have a way to know exactly how much DOM memory is
> retained by v8 wrappers.
> 
> The original plan was to v8::LowMemoryNotification when partitionAlloc is
using
> too much mem.
> As this would force major gc multiple times, I received concerns from v8 team.
> 
> The next plan was to implement an alternative api to tell this to v8. (
> https://code.google.com/p/v8/issues/detail?id=3338&thanks=3338&ts=1400591978 )
> Before that we wanted to know if it really worth adding a new api.

Yes, that's the plan. And I think the above result from Dromaeo is justifying
the need for the memory-limit V8 API.

Would you summarize the result in the V8 bug?

[Hannes Payer (out of office), 2014-05-28 11:00:06]

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:110:
Partitions::addDOMMemoryUsage(partitionAllocGetSize(ptr));
Is this call here causing the reported performance degradation on Dromeo?

[jochen (gone - plz use gerrit), 2014-05-28 12:00:28]

An alternative would be to add an API where v8 can query the current memory
consumption, e.g. by accessing some integer value directly, when v8 needs to
know that information.

[haraken, 2014-05-28 12:05:59]

On 2014/05/28 12:00:28, jochen wrote:
> An alternative would be to add an API where v8 can query the current memory
> consumption, e.g. by accessing some integer value directly, when v8 needs to
> know that information.

I'd prefer sending the current memory usage from the embedder side to V8.
Otherwise, the embedder cannot do anything when it reaches a memory limit (this
is our current situation).

[eseidel, 2014-05-28 22:37:53]

I'm very exited about this change!  However I'm not the right person to approve
it.  If jochen, danno, cevans, tsepez or the like are in favor, you certanly
have my l-g-t-m.

[kouhei (in TOK), 2014-05-29 03:42:32]

I summarized the current situation on
https://code.google.com/p/chromium/issues/detail?id=368406#c45 .
I'd appreciate if you would comment on it. (esp. if we are allowd to use
adjustAmountOfExternalAllocatedMemory to report all DOM memory usage)

[eseidel, 2014-05-29 05:49:48]

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:110:
Partitions::addDOMMemoryUsage(partitionAllocGetSize(ptr));
Why would you want to report it during Node alloc?  Why not report it to v8
every time partitionAlloc allocs a new superpage or some other semi-infrequent
allocation from partitionAlloc directly?  Doesn't V8 want to know about memory
pressure from the entire renderer?

[kouhei (in TOK), 2014-05-29 06:02:22]

On 2014/05/29 05:49:48, eseidel wrote:
> https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp
> File Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/301743006/diff/1/Source/core/dom/Node.cpp#new...
> Source/core/dom/Node.cpp:110:
> Partitions::addDOMMemoryUsage(partitionAllocGetSize(ptr));
> Why would you want to report it during Node alloc?  Why not report it to v8
> every time partitionAlloc allocs a new superpage or some other semi-infrequent
> allocation from partitionAlloc directly?  Doesn't V8 want to know about memory
> pressure from the entire renderer?

No, I don't think this is so bad. This is just modifying a global counter and
actual report is deferred to task end.
I think partitionAllocGetSize overhead is causing some slowness. I'll polish
this by integrating the counter to partitionAlloc() itself to avoid unnecessary
duplicated computation.

[kouhei (in TOK), 2014-06-04 12:54:02]

I tested this along with jochen's v8 patch:
https://codereview.chromium.org/310393003
and it looks like we have no regression.

The full dromaeo result is here:
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0Apo-0Snm1_rFdFBhW...

Given those results, I'm thinking of continuing with Patch Set 2 approach. Any
thoughts?

[haraken, 2014-06-04 12:58:01]

On 2014/06/04 12:54:02, kouhei wrote:
> I tested this along with jochen's v8 patch:
> https://codereview.chromium.org/310393003
> and it looks like we have no regression.
> 
> The full dromaeo result is here:
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0Apo-0Snm1_rFdFBhW...
> 
> Given those results, I'm thinking of continuing with Patch Set 2 approach. Any
> thoughts?

I'm not sure how jochen's CL is helpful to improve the performance of kouhei's
CL (because kouhei's CL is calling AdjustAmountOfMemory only at the end of each
event loop; so the overhead of AdjustAmountOfMemory won't matter), but I'm fine
with the patch set 2 if there is no regression in Dromaeo.

[esprehn, 2014-06-05 01:04:26]

Just ask PartitionAlloc for the size of the partition, you don't need all the
book keeping.

https://codereview.chromium.org/301743006/diff/40001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/301743006/diff/40001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:111:
Partitions::addDOMMemoryUsage(1024);//partitionAllocGetSize(ptr));
This still seems wrong, why aren't you just reporting the size of the all memory
in the node partition instead?

You shouldn't need to put this code inside new/delete.

[kouhei (in TOK), 2014-06-05 02:12:48]

On 2014/06/04 12:58:01, haraken wrote:
> On 2014/06/04 12:54:02, kouhei wrote:
> > I tested this along with jochen's v8 patch:
> > https://codereview.chromium.org/310393003
> > and it looks like we have no regression.
> > 
> > The full dromaeo result is here:
> >
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0Apo-0Snm1_rFdFBhW...
> > 
> > Given those results, I'm thinking of continuing with Patch Set 2 approach.
Any
> > thoughts?
> 
> I'm not sure how jochen's CL is helpful to improve the performance of kouhei's
> CL (because kouhei's CL is calling AdjustAmountOfMemory only at the end of
each
> event loop; so the overhead of AdjustAmountOfMemory won't matter), but I'm
fine
> with the patch set 2 if there is no regression in Dromaeo.

It looks like I did something wrong in the first benchmark.

I rerun Dromaeo with and without this CL:
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0Apo-0Snm1_rFdFBhW...
The result is that we don't have a large performance impact from this CL, and
jochen's CL is unrelated to performance impact of this CL.

[kouhei (in TOK), 2014-06-05 02:17:20]

On 2014/06/05 01:04:26, esprehn wrote:
> Just ask PartitionAlloc for the size of the partition, you don't need all the
> book keeping.
> 
> https://codereview.chromium.org/301743006/diff/40001/Source/core/dom/Node.cpp
> File Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/301743006/diff/40001/Source/core/dom/Node.cpp...
> Source/core/dom/Node.cpp:111:
> Partitions::addDOMMemoryUsage(1024);//partitionAllocGetSize(ptr));
> This still seems wrong, why aren't you just reporting the size of the all
memory
> in the node partition instead?
> 
> You shouldn't need to put this code inside new/delete.

Yes. I will move this bookkeeping from Node::new/delete to inside
PartitionAlloc.
Currently we don't have a precise usage metric inside PartitionAlloc that can be
queried fast enough, so I'll add a simple counter in partition{Alloc,Free}.

[kouhei (in TOK), 2014-06-05 04:24:08]

PTAL.
I moved the counter to PartitionAllocRoot and I don't see regression from
Dromaeo.

On 2014/06/05 02:17:20, kouhei wrote:
> On 2014/06/05 01:04:26, esprehn wrote:
> > Just ask PartitionAlloc for the size of the partition, you don't need all
the
> > book keeping.
> > 
> >
https://codereview.chromium.org/301743006/diff/40001/Source/core/dom/Node.cpp
> > File Source/core/dom/Node.cpp (right):
> > 
> >
>
https://codereview.chromium.org/301743006/diff/40001/Source/core/dom/Node.cpp...
> > Source/core/dom/Node.cpp:111:
> > Partitions::addDOMMemoryUsage(1024);//partitionAllocGetSize(ptr));
> > This still seems wrong, why aren't you just reporting the size of the all
> memory
> > in the node partition instead?
> > 
> > You shouldn't need to put this code inside new/delete.
> 
> Yes. I will move this bookkeeping from Node::new/delete to inside
> PartitionAlloc.
> Currently we don't have a precise usage metric inside PartitionAlloc that can
be
> queried fast enough, so I'll add a simple counter in partition{Alloc,Free}.

Done.

[esprehn, 2014-06-05 04:34:30]

This looks good to me, but I think you need someone like cevans or tsepez to
review the Partition alloc change.

[haraken, 2014-06-05 04:49:14]

Looks good to me as well if PartitionAlloc guys and V8 guys like it.

[jochen (gone - plz use gerrit), 2014-06-05 06:54:50]

lgtm

[Hannes Payer (out of office), 2014-06-05 08:29:51]

lgtm

[kouhei (in TOK), 2014-06-05 08:37:58]

cevans, tsepez: Would you take a look at PartitionAlloc change? Thanks!

[Tom Sepez, 2014-06-05 17:50:52]

The changes themselves LGTM, but when adding code to this routines, we have to
be careful that some stingy optimizer hasn't decided that the new code is too
large to inline ... which would show up as a performance hit.

[Tom Sepez, 2014-06-05 17:52:01]

https://codereview.chromium.org/301743006/diff/80001/Source/wtf/PartitionAlloc.h
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/301743006/diff/80001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAlloc.h:509: root->totalAllocationSize -= bucketSize;
nit: assert you don't underflow here.

[Tom Sepez, 2014-06-05 17:58:08]

https://codereview.chromium.org/301743006/diff/80001/Source/bindings/v8/V8GCC...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/301743006/diff/80001/Source/bindings/v8/V8GCC...
Source/bindings/v8/V8GCController.cpp:450:
isolate->AdjustAmountOfExternalAllocatedMemory(diff);
nit: down the road, you'd probably be happier with an V8 API that took an
absolute.

[Chris Evans, 2014-06-05 18:00:26]

https://codereview.chromium.org/301743006/diff/80001/Source/wtf/PartitionAlloc.h
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/301743006/diff/80001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAlloc.h:468: root->totalAllocationSize += bucket->slotSize;
My gut reaction is I don't like this. It adds overhead to the (insanely
optimized) fast paths. Can we find a solution that is slow path only? Maybe the
number of active + full pages (i.e. resident system pages) is a good
approximation.

In fact it might even be better, as it will cause gc pressure on v8 if we have a
low allocation count but high fragmentation.

[Chris Evans, 2014-06-05 18:01:39]

On 2014/06/05 18:00:26, Chris Evans wrote:
>
https://codereview.chromium.org/301743006/diff/80001/Source/wtf/PartitionAlloc.h
> File Source/wtf/PartitionAlloc.h (right):
> 
>
https://codereview.chromium.org/301743006/diff/80001/Source/wtf/PartitionAllo...
> Source/wtf/PartitionAlloc.h:468: root->totalAllocationSize +=
bucket->slotSize;
> My gut reaction is I don't like this. It adds overhead to the (insanely
> optimized) fast paths. Can we find a solution that is slow path only? Maybe
the
> number of active + full pages (i.e. resident system pages) is a good
> approximation.
> 
> In fact it might even be better, as it will cause gc pressure on v8 if we have
a
> low allocation count but high fragmentation.

i.e. not lgtm

[kouhei (in TOK), 2014-06-06 11:24:28]

Thanks for your comments.

I modified the patch to count number of non-free pages so that we don't slow
down the fast path.

cevans, tsepez: PTAL.
jochen, hpayer: Would this change of counter granularity acceptable to you?

https://codereview.chromium.org/301743006/diff/100001/Source/bindings/v8/V8GC...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/301743006/diff/100001/Source/bindings/v8/V8GC...
Source/bindings/v8/V8GCController.cpp:449: fprintf(stderr, "currentUsage:
%zu\n", currentUsage);
I will remove this printf.

[Chris Evans, 2014-06-06 18:30:50]

https://codereview.chromium.org/301743006/diff/100001/Source/platform/Partiti...
File Source/platform/Partitions.h (right):

https://codereview.chromium.org/301743006/diff/100001/Source/platform/Partiti...
Source/platform/Partitions.h:47: static size_t currentDOMMemoryUsage() { return
m_objectModelAllocator.root()->totalSizeOfNonFreePages; }
I don't see any PartitionAlloc.h change in this latest patch? It looks like we
need one?

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:115: root->totalSizeOfNonFreePages = 0;
I think we should name this totalSizeOfCommittedPages, it's clearer what it is.

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:682: root->totalSizeOfNonFreePages += commitSize;
I think this is the right model: adjusting the 

However, we've missed the adjustment in the other call to recommitSystemPages in
this file. I'd be ok with doing this more safely: do that actual size adjustment
in a helper function that does the adjustment and then calls
recommitSystemPages. We could then re-use that code. If it means the need to
pass the root around more, I'm fine with that.

Also, we've missed an adjustment around another decommitSystemPages call. We
need to get them all :-) Same solution applies.

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:719: root->totalSizeOfNonFreePages -= pageSize;
I do not think this is the correct place to do this. The page is empty but is
still committed. I think we might do it in a wrapper around the call to
decommitSystemPages(), same idea as above. Again, this would capture both places
we need to do it.

[kouhei (in TOK), 2014-06-09 05:48:22]

PTAL.

https://codereview.chromium.org/301743006/diff/100001/Source/bindings/v8/V8GC...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/301743006/diff/100001/Source/bindings/v8/V8GC...
Source/bindings/v8/V8GCController.cpp:449: fprintf(stderr, "currentUsage:
%zu\n", currentUsage);
On 2014/06/06 11:24:28, kouhei wrote:
> I will remove this printf.

Done.

https://codereview.chromium.org/301743006/diff/100001/Source/platform/Partiti...
File Source/platform/Partitions.h (right):

https://codereview.chromium.org/301743006/diff/100001/Source/platform/Partiti...
Source/platform/Partitions.h:47: static size_t currentDOMMemoryUsage() { return
m_objectModelAllocator.root()->totalSizeOfNonFreePages; }
On 2014/06/06 18:30:50, Chris Evans wrote:
> I don't see any PartitionAlloc.h change in this latest patch? It looks like we
> need one?

Done. Sorry I reverted it by mistake.

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:115: root->totalSizeOfNonFreePages = 0;
On 2014/06/06 18:30:50, Chris Evans wrote:
> I think we should name this totalSizeOfCommittedPages, it's clearer what it
is.

Done.

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:682: root->totalSizeOfNonFreePages += commitSize;
On 2014/06/06 18:30:50, Chris Evans wrote:
> I think this is the right model: adjusting the 
> 
> However, we've missed the adjustment in the other call to recommitSystemPages
in
> this file. I'd be ok with doing this more safely: do that actual size
adjustment
> in a helper function that does the adjustment and then calls
> recommitSystemPages. We could then re-use that code. If it means the need to
> pass the root around more, I'm fine with that.
> 
> Also, we've missed an adjustment around another decommitSystemPages call. We
> need to get them all :-) Same solution applies.

Done. I added partition{Re,De}commitPages wrapper func.

https://codereview.chromium.org/301743006/diff/100001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:719: root->totalSizeOfNonFreePages -= pageSize;
On 2014/06/06 18:30:50, Chris Evans wrote:
> I do not think this is the correct place to do this. The page is empty but is
> still committed. I think we might do it in a wrapper around the call to
> decommitSystemPages(), same idea as above. Again, this would capture both
places
> we need to do it.
Removed this, and moved counter decrement into partitionDecommitSystemPages().

[kouhei (in TOK), 2014-06-09 10:13:45]

https://codereview.chromium.org/301743006/diff/120001/Source/bindings/v8/V8GC...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/301743006/diff/120001/Source/bindings/v8/V8GC...
Source/bindings/v8/V8GCController.cpp:449: ssize_t diff =
static_cast<ssize_t>(currentUsage) -
static_cast<ssize_t>(lastUsageReportedToV8);
FIXME: no ssize_t in Windows :(

[Chris Evans, 2014-06-09 17:11:34]

On 2014/06/09 10:13:47, kouhei wrote:

*Partition* LGTM -- nice and generic, and only affects the slow path :)

medium-sized nit: there's no changes to the tests in PartitionAllocTest.cpp.
"FreeCache" might be a good place to add a few EXPECTs because it
deterministically forces a partition page to get decommitted at a very clear
point in the test.

[kouhei (in TOK), 2014-06-10 03:55:12]

danno, hpayer, jochen: Would you take a look? I'd like to land this change if
this looks ok to you.
hapyer: Since your last lgtm, I changed the code so it will count number of
committed pages rather than specified alloc size for lower overhead in
PartitionAlloc.

On 2014/06/09 17:11:34, Chris Evans wrote:
> On 2014/06/09 10:13:47, kouhei wrote:
> 
> *Partition* LGTM -- nice and generic, and only affects the slow path :)
Thanks!

> medium-sized nit: there's no changes to the tests in PartitionAllocTest.cpp.
> "FreeCache" might be a good place to add a few EXPECTs because it
> deterministically forces a partition page to get decommitted at a very clear
> point in the test.
Ack. I'll add a test before landing.

[jochen (gone - plz use gerrit), 2014-06-10 10:11:50]

lgtm

[Hannes Payer (out of office), 2014-06-11 13:38:10]

lgtm

[kouhei (in TOK), 2014-06-12 07:19:08]

I'd like to try landing this.
danno: Would you comment on this if you have any concerns? Thanks.

[kouhei (in TOK), 2014-06-12 07:19:22]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-12 07:20:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/301743006/140001

[commit-bot: I haz the power, 2014-06-12 07:44:20]

Change committed as 176006