Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(39)

Issue 183113003: PartitionAlloc: support in-place resize of directly mapped allocation (Closed)

Created:
6 years, 9 months ago by Jens Widell
Modified:
6 years, 9 months ago
Reviewers:
Chris Evans
CC:
blink-reviews, loislo+blink_chromium.org, yurys+blink_chromium.org, abarth-chromium, adamk+blink_chromium.org, Inactive, Mikhail
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

PartitionAlloc: support in-place resize of directly mapped allocation In partitionReallocGeneric(), if both the current size and the new size are in the direct mapped range, attempt to resize the allocation in place. When downsizing, this is done by decommitting pages and making them inaccessible. When upsizing a previously downsized allocation to a size not larger than its original size, we make the pages accessible again. R=cevans@chromium.org BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168633

Patch Set 1 #

Total comments: 2

Patch Set 2 : fix various issues #

Total comments: 12

Patch Set 3 : assorted simple fixes #

Patch Set 4 : redesign a bit as suggested #

Total comments: 7

Patch Set 5 : addressing some more comments #

Total comments: 3

Patch Set 6 : nits addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -7 lines) Patch
M Source/wtf/PartitionAlloc.h View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M Source/wtf/PartitionAlloc.cpp View 1 2 3 4 5 6 chunks +79 lines, -6 lines 0 comments Download
M Source/wtf/PartitionAllocTest.cpp View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (0 generated)
Jens Widell
A working implementation (I think) but a bit of an RFC on the design. The ...
6 years, 9 months ago (2014-02-27 14:14:26 UTC) #1
Jens Widell
https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cpp#newcode589 Source/wtf/PartitionAlloc.cpp:589: size_t unmapSize = page->bucket->slotSize; This would need adjusting to ...
6 years, 9 months ago (2014-02-27 17:02:55 UTC) #2
Jens Widell
Finding flaws. :-) https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/1/Source/wtf/PartitionAlloc.cpp#newcode784 Source/wtf/PartitionAlloc.cpp:784: setSystemPagesInaccessible(realPtr + newSize, kSystemPageSize); Ought to ...
6 years, 9 months ago (2014-02-27 17:10:53 UTC) #3
Chris Evans
Sorry for the slow response time. Some initial comments! https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAlloc.cpp#newcode527 Source/wtf/PartitionAlloc.cpp:527: ...
6 years, 9 months ago (2014-03-05 07:57:03 UTC) #4
Jens Widell
https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAlloc.cpp#newcode602 Source/wtf/PartitionAlloc.cpp:602: bucket->freePagesHead = reinterpret_cast<PartitionPage*>(ptr + mapSize); On 2014/03/05 07:57:04, Chris ...
6 years, 9 months ago (2014-03-05 08:11:42 UTC) #5
Chris Evans
On 2014/03/05 08:11:42, Jens Lindström wrote: > https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAlloc.cpp > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/183113003/diff/20001/Source/wtf/PartitionAlloc.cpp#newcode602 ...
6 years, 9 months ago (2014-03-05 08:17:27 UTC) #6
Jens Widell
Patch updated to address all the comments so far (I think.) Introduced a "struct PartitionDirectMapExtent" ...
6 years, 9 months ago (2014-03-05 12:03:09 UTC) #7
Chris Evans
Getting close! https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAlloc.cpp#newcode791 Source/wtf/PartitionAlloc.cpp:791: PartitionPage* page = partitionPointerToPage(ptr); Pass in page ...
6 years, 9 months ago (2014-03-06 05:55:13 UTC) #8
Jens Widell
https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/50001/Source/wtf/PartitionAlloc.cpp#newcode797 Source/wtf/PartitionAlloc.cpp:797: if (newSize == bucket->slotSize) On 2014/03/06 05:55:13, Chris Evans ...
6 years, 9 months ago (2014-03-06 06:03:04 UTC) #9
Jens Widell
On 2014/03/06 05:55:13, Chris Evans wrote: > Getting close! Thanks for great feedback! :-) Patch ...
6 years, 9 months ago (2014-03-06 06:23:55 UTC) #10
Chris Evans
LGTM 2 nits; do with them as you please. No need to re-review. https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAlloc.cpp File ...
6 years, 9 months ago (2014-03-06 06:30:20 UTC) #11
Chris Evans
One more nit. https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAlloc.cpp File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAlloc.cpp#newcode847 Source/wtf/PartitionAlloc.cpp:847: if (partitionBucketIsDirectMapped(page->bucket)) { Nit: could add ...
6 years, 9 months ago (2014-03-06 06:31:22 UTC) #12
Jens Widell
On 2014/03/06 06:31:22, Chris Evans wrote: > One more nit. > > https://codereview.chromium.org/183113003/diff/60001/Source/wtf/PartitionAlloc.cpp > File ...
6 years, 9 months ago (2014-03-06 06:38:22 UTC) #13
Jens Widell
The CQ bit was checked by jl@opera.com
6 years, 9 months ago (2014-03-06 06:59:44 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/183113003/80001
6 years, 9 months ago (2014-03-06 06:59:55 UTC) #15
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-06 08:13:57 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink
6 years, 9 months ago (2014-03-06 08:13:57 UTC) #17
Jens Widell
The CQ bit was checked by jl@opera.com
6 years, 9 months ago (2014-03-06 09:05:10 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/183113003/80001
6 years, 9 months ago (2014-03-06 09:05:22 UTC) #19
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-06 10:15:12 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink
6 years, 9 months ago (2014-03-06 10:15:13 UTC) #21
Jens Widell
The CQ bit was checked by jl@opera.com
6 years, 9 months ago (2014-03-06 11:57:25 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/183113003/80001
6 years, 9 months ago (2014-03-06 11:57:45 UTC) #23
commit-bot: I haz the power
6 years, 9 months ago (2014-03-06 13:07:15 UTC) #24
Message was sent while issue was closed.
Change committed as 168633

Powered by Google App Engine
This is Rietveld 408576698