Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(332)

Issue 818253005: Oilpan: Query stack frame register instead of manual bookkeeping (Closed)

Created:
5 years, 11 months ago by kouhei (in TOK)
Modified:
5 years, 11 months ago
Reviewers:
oilpan-reviews, haraken, sof
CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium)
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/remotes/origin/master
Project:
blink
Visibility:
Public.

Description

Oilpan: Query stack frame register instead of manual bookkeeping This CL introduce new class StackFrameDepth to keep track of current stack frame depth. On supported ABI/compiler, StackFrameDepth queries current stack frame base register (e.g. %rbp on X86_64), skipping manual bookkeeping. This improves Oilpan marking time by 17% on BlinkGC.stress-large-SVGLengthList.html. TEST=PerformanceTests/BlinkGC/stress-large-SVGLengthList.html BUG=420515 R=haraken@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188108 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188585

Patch Set 1 #

Total comments: 3

Patch Set 2 : fix linux #

Patch Set 3 : fix linux2 #

Patch Set 4 : ensure m_traceDepth is initialized for CountingVisitortests #

Patch Set 5 : add PLATFORM_EXPORTg #

Patch Set 6 : ENABLE(STACKFRAME_POINTER_ESTIMATION) #

Total comments: 15

Patch Set 7 : haraken review #

Total comments: 1

Patch Set 8 : rename #

Patch Set 9 : fix debug ASSERTS from CachedMatchedProperties #

Patch Set 10 : need more depth to cause non-eager trace for arm32 #

Total comments: 3

Patch Set 11 : review #

Patch Set 12 : rebased #

Patch Set 13 : move preGC #

Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -19 lines) Patch
M Source/platform/heap/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/platform/heap/Heap.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -0 lines 0 comments Download
M Source/platform/heap/HeapTest.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M Source/platform/heap/InlinedGlobalMarkingVisitor.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -3 lines 0 comments Download
A Source/platform/heap/StackFrameDepth.h View 1 2 3 4 5 6 1 chunk +52 lines, -0 lines 0 comments Download
A Source/platform/heap/StackFrameDepth.cpp View 1 2 3 4 5 6 7 1 chunk +30 lines, -0 lines 0 comments Download
M Source/platform/heap/Visitor.h View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +19 lines, -14 lines 0 comments Download
M Source/platform/heap/Visitor.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M Source/platform/heap/blink_heap.gypi View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 43 (12 generated)
kouhei (in TOK)
5 years, 11 months ago (2015-01-07 03:59:02 UTC) #2
haraken
Looks like you need to fix bot failures. https://codereview.chromium.org/818253005/diff/1/Source/platform/heap/StackFrameDepth.h File Source/platform/heap/StackFrameDepth.h (right): https://codereview.chromium.org/818253005/diff/1/Source/platform/heap/StackFrameDepth.h#newcode73 Source/platform/heap/StackFrameDepth.h:73: return ...
5 years, 11 months ago (2015-01-07 05:04:44 UTC) #4
kouhei (in TOK)
https://codereview.chromium.org/818253005/diff/1/Source/platform/heap/StackFrameDepth.h File Source/platform/heap/StackFrameDepth.h (right): https://codereview.chromium.org/818253005/diff/1/Source/platform/heap/StackFrameDepth.h#newcode73 Source/platform/heap/StackFrameDepth.h:73: return reinterpret_cast<uintptr_t>(&dummy); On 2015/01/07 05:04:44, haraken wrote: > > ...
5 years, 11 months ago (2015-01-07 05:06:39 UTC) #5
haraken
https://codereview.chromium.org/818253005/diff/1/Source/platform/heap/StackFrameDepth.h File Source/platform/heap/StackFrameDepth.h (right): https://codereview.chromium.org/818253005/diff/1/Source/platform/heap/StackFrameDepth.h#newcode73 Source/platform/heap/StackFrameDepth.h:73: return reinterpret_cast<uintptr_t>(&dummy); On 2015/01/07 05:06:39, kouhei wrote: > On ...
5 years, 11 months ago (2015-01-07 05:09:51 UTC) #6
kouhei (in TOK)
Removed m_depth path. On 2015/01/07 05:09:51, haraken wrote: > https://codereview.chromium.org/818253005/diff/1/Source/platform/heap/StackFrameDepth.h > File Source/platform/heap/StackFrameDepth.h (right): > ...
5 years, 11 months ago (2015-01-07 07:26:12 UTC) #7
haraken
Mostly looks good. https://codereview.chromium.org/818253005/diff/90001/Source/platform/heap/StackFrameDepth.cpp File Source/platform/heap/StackFrameDepth.cpp (right): https://codereview.chromium.org/818253005/diff/90001/Source/platform/heap/StackFrameDepth.cpp#newcode17 Source/platform/heap/StackFrameDepth.cpp:17: void StackFrameDepth::configureLimit() Can we add NEVER_INLINE ...
5 years, 11 months ago (2015-01-08 01:14:23 UTC) #8
kouhei (in TOK)
PTAL https://codereview.chromium.org/818253005/diff/90001/Source/platform/heap/StackFrameDepth.cpp File Source/platform/heap/StackFrameDepth.cpp (right): https://codereview.chromium.org/818253005/diff/90001/Source/platform/heap/StackFrameDepth.cpp#newcode17 Source/platform/heap/StackFrameDepth.cpp:17: void StackFrameDepth::configureLimit() On 2015/01/08 01:14:22, haraken wrote: > ...
5 years, 11 months ago (2015-01-08 03:29:55 UTC) #9
haraken
LGTM https://codereview.chromium.org/818253005/diff/110001/Source/platform/heap/StackFrameDepth.cpp File Source/platform/heap/StackFrameDepth.cpp (right): https://codereview.chromium.org/818253005/diff/110001/Source/platform/heap/StackFrameDepth.cpp#newcode11 Source/platform/heap/StackFrameDepth.cpp:11: NEVER_INLINE static uintptr_t estimateStackFrameBaseOnCallee(const char* dummy) estimateStackFrameBaseOnCallee => ...
5 years, 11 months ago (2015-01-08 04:09:23 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/818253005/130001
5 years, 11 months ago (2015-01-08 04:18:04 UTC) #12
commit-bot: I haz the power
Exceeded time limit waiting for builds to trigger.
5 years, 11 months ago (2015-01-08 06:18:41 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/818253005/130001
5 years, 11 months ago (2015-01-08 06:19:18 UTC) #16
commit-bot: I haz the power
Exceeded time limit waiting for builds to trigger.
5 years, 11 months ago (2015-01-08 08:20:17 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/818253005/130001
5 years, 11 months ago (2015-01-08 10:54:06 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/818253005/130001
5 years, 11 months ago (2015-01-09 01:07:32 UTC) #23
kouhei (in TOK)
Committed patchset #8 (id:130001) manually as 188108 (presubmit successful).
5 years, 11 months ago (2015-01-09 03:50:27 UTC) #25
keishi
HeapTest.TraceDeepEagerly is failing on WebKit Android (Nexus4) bot. http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29/builds/25031 The blame range is 311ce62..5b9eff1 so ...
5 years, 11 months ago (2015-01-09 06:02:50 UTC) #26
sof
Does this scheme work for the thread-specific marking done during weak processing?
5 years, 11 months ago (2015-01-12 09:11:03 UTC) #28
haraken
On 2015/01/12 09:11:03, sof wrote: > Does this scheme work for the thread-specific marking done ...
5 years, 11 months ago (2015-01-12 09:16:13 UTC) #29
sof
On 2015/01/12 09:16:13, haraken wrote: > On 2015/01/12 09:11:03, sof wrote: > > Does this ...
5 years, 11 months ago (2015-01-12 09:20:20 UTC) #30
sof
On 2015/01/12 09:20:20, sof wrote: > On 2015/01/12 09:16:13, haraken wrote: > > On 2015/01/12 ...
5 years, 11 months ago (2015-01-12 09:21:57 UTC) #31
haraken
On 2015/01/12 09:21:57, sof wrote: > On 2015/01/12 09:20:20, sof wrote: > > On 2015/01/12 ...
5 years, 11 months ago (2015-01-12 09:28:26 UTC) #32
sof
On 2015/01/12 09:28:26, haraken wrote: > On 2015/01/12 09:21:57, sof wrote: > > On 2015/01/12 ...
5 years, 11 months ago (2015-01-12 09:32:44 UTC) #33
kouhei (in TOK)
A revert of this CL (patchset #8 id:130001) has been created in https://codereview.chromium.org/851563003/ by kouhei@chromium.org. ...
5 years, 11 months ago (2015-01-13 01:18:12 UTC) #34
kouhei (in TOK)
PTAL On 2015/01/12 09:28:26, haraken wrote: > On 2015/01/12 09:21:57, sof wrote: > > On ...
5 years, 11 months ago (2015-01-14 03:37:03 UTC) #35
kouhei (in TOK)
On 2015/01/14 03:37:03, kouhei wrote: > PTAL > > On 2015/01/12 09:28:26, haraken wrote: > ...
5 years, 11 months ago (2015-01-14 03:41:50 UTC) #36
haraken
LGTM, but I want to have Sigbjorn to take another look. https://codereview.chromium.org/818253005/diff/170001/Source/platform/heap/Visitor.h File Source/platform/heap/Visitor.h (right): ...
5 years, 11 months ago (2015-01-14 05:29:57 UTC) #37
sof
lgtm https://codereview.chromium.org/818253005/diff/170001/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/818253005/diff/170001/Source/platform/heap/Heap.cpp#newcode2311 Source/platform/heap/Heap.cpp:2311: s_markingVisitor->configureEagerTraceLimit(); Move this to preGC() ? https://codereview.chromium.org/818253005/diff/170001/Source/platform/heap/Visitor.h File ...
5 years, 11 months ago (2015-01-14 07:03:43 UTC) #38
kouhei (in TOK)
Thanks! On 2015/01/14 07:03:43, sof wrote: > lgtm > > https://codereview.chromium.org/818253005/diff/170001/Source/platform/heap/Heap.cpp > File Source/platform/heap/Heap.cpp (right): ...
5 years, 11 months ago (2015-01-14 07:34:27 UTC) #39
kouhei (in TOK)
> On 2015/01/14 07:03:43, sof wrote: > > lgtm > > > > > https://codereview.chromium.org/818253005/diff/170001/Source/platform/heap/Heap.cpp ...
5 years, 11 months ago (2015-01-15 05:58:05 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/818253005/230001
5 years, 11 months ago (2015-01-19 02:10:44 UTC) #42
commit-bot: I haz the power
5 years, 11 months ago (2015-01-19 05:27:45 UTC) #43
Message was sent while issue was closed.
Committed patchset #13 (id:230001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188585

Powered by Google App Engine
This is Rietveld 408576698