Extend locking of GCInfo table to include slot allocation.

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

[sof, 2015-01-10 08:26:56]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-01-10 08:26:57]

Please take a lock.

[haraken, 2015-01-10 08:37:24]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-01-10 08:37:25]

LGTM, thanks for the fix.

https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor...
File Source/platform/heap/Visitor.cpp (right):

https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.cpp:46: bool
GCInfoTable::allocateGCInfoSlot(size_t& gcInfoIndexSlot)

Nit: I'd slightly prefer a raw pointer for an output parameter. (That's a
convention in the chromium side, and no convention in the Blink side.)

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...
Source/platform/heap/Visitor.h:887: s_gcInfoTable[gcInfoIndex] = &gcInfo;       
  \

Can we do this assignment in allocateGCInfoSlot? Probably we can rename it to
ensureGCInfoIndex or something.

[sof, 2015-01-10 08:42:46]

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...
Source/platform/heap/Visitor.h:887: s_gcInfoTable[gcInfoIndex] = &gcInfo;       
  \
On 2015/01/10 08:37:24, haraken wrote:
> 
> Can we do this assignment in allocateGCInfoSlot? Probably we can rename it to
> ensureGCInfoIndex or something.

I suppose, but I don't see the overall benefit.

[haraken, 2015-01-10 08:48:45]

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...
Source/platform/heap/Visitor.h:887: s_gcInfoTable[gcInfoIndex] = &gcInfo;       
  \
On 2015/01/10 08:42:46, sof wrote:
> On 2015/01/10 08:37:24, haraken wrote:
> > 
> > Can we do this assignment in allocateGCInfoSlot? Probably we can rename it
to
> > ensureGCInfoIndex or something.
> 
> I suppose, but I don't see the overall benefit.

up to you :)

[sof, 2015-01-10 08:54:23]

Thanks for the quick (and OOO) review :)

https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor...
File Source/platform/heap/Visitor.cpp (right):

https://codereview.chromium.org/834673008/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.cpp:46: bool
GCInfoTable::allocateGCInfoSlot(size_t& gcInfoIndexSlot)
On 2015/01/10 08:37:24, haraken wrote:
> 
> Nit: I'd slightly prefer a raw pointer for an output parameter. (That's a
> convention in the chromium side, and no convention in the Blink side.)

Done.

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...
Source/platform/heap/Visitor.h:887: s_gcInfoTable[gcInfoIndex] = &gcInfo;       
  \
On 2015/01/10 08:48:45, haraken wrote:
> On 2015/01/10 08:42:46, sof wrote:
> > On 2015/01/10 08:37:24, haraken wrote:
> > > 
> > > Can we do this assignment in allocateGCInfoSlot? Probably we can rename it
> to
> > > ensureGCInfoIndex or something.
> > 
> > I suppose, but I don't see the overall benefit.
> 
> up to you :)

Done.

[sof, 2015-01-10 09:04:01]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-01-10 09:04:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/834673008/20001

[commit-bot: I haz the power, 2015-01-10 12:12:01]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188184