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

Issue 834673008: Extend locking of GCInfo table to include slot allocation. (Closed)

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

Description

Extend locking of GCInfo table to include slot allocation. To prevent threads from allocating duplicate slots for a GCInfo descriptor, impose a lock on slot allocation. With the lock held, avoid duplicates by re-checking if the slot hasn't been initialized. R=haraken BUG=357427 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188184

Patch Set 1 #

Total comments: 6

Patch Set 2 : Adjust position of slot initialization #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -27 lines) Patch
M Source/platform/heap/Visitor.h View 1 3 chunks +9 lines, -8 lines 0 comments Download
M Source/platform/heap/Visitor.cpp View 1 2 chunks +19 lines, -19 lines 0 comments Download

Messages

Total messages: 10 (3 generated)
sof
Please take a lock.
5 years, 11 months ago (2015-01-10 08:26:57 UTC) #2
haraken
LGTM, thanks for the fix. https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.cpp File Source/platform/heap/Visitor.cpp (right): https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.cpp#newcode46 Source/platform/heap/Visitor.cpp:46: bool GCInfoTable::allocateGCInfoSlot(size_t& gcInfoIndexSlot) Nit: ...
5 years, 11 months ago (2015-01-10 08:37:25 UTC) #4
sof
https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.h File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.h#newcode887 Source/platform/heap/Visitor.h:887: s_gcInfoTable[gcInfoIndex] = &gcInfo; \ On 2015/01/10 08:37:24, haraken wrote: ...
5 years, 11 months ago (2015-01-10 08:42:46 UTC) #5
haraken
https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.h File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.h#newcode887 Source/platform/heap/Visitor.h:887: s_gcInfoTable[gcInfoIndex] = &gcInfo; \ On 2015/01/10 08:42:46, sof wrote: ...
5 years, 11 months ago (2015-01-10 08:48:45 UTC) #6
sof
Thanks for the quick (and OOO) review :) https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.cpp File Source/platform/heap/Visitor.cpp (right): https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor.cpp#newcode46 Source/platform/heap/Visitor.cpp:46: bool ...
5 years, 11 months ago (2015-01-10 08:54:23 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/834673008/20001
5 years, 11 months ago (2015-01-10 09:04:51 UTC) #9
commit-bot: I haz the power
5 years, 11 months ago (2015-01-10 12:12:01 UTC) #10
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188184

Powered by Google App Engine
This is Rietveld 408576698