Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1312963002:
Fix race at GCInfo::index. (Closed)
DescriptionFix race at GCInfo::index.
This is a fix for the data race in GCInfoAtBase::index.
The GCInfo is defined as static local and this causes race while
allocating new objects. This CL reduces the initialization time by
using function pointer to get the class name instead of string parsing.
BUG=524237
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201223
Patch Set 1 #Patch Set 2 : Remove static locals, leave GCInfo unsafe. #
Messages
Total messages: 30 (9 generated)
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM
ssid@chromium.org changed reviewers: - haraken@chromium.org
The CQ bit was checked by ssid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312963002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Could someone explain why these race bugs are surfacing now/recently?
On 2015/08/25 05:30:44, sof wrote: > Could someone explain why these race bugs are surfacing now/recently? I asked the same question offline. ssid's guess is that ssid's change increased the overhead of TypenameStringTrait<T>::get() and made the threading race more likely.
On 2015/08/25 05:37:38, haraken wrote: > On 2015/08/25 05:30:44, sof wrote: > > Could someone explain why these race bugs are surfacing now/recently? > > I asked the same question offline. ssid's guess is that ssid's change increased > the overhead of TypenameStringTrait<T>::get() and made the threading race more > likely. I haven't looked into details, but it cannot be TypenameStringTrait<T>::get instead?
On 2015/08/25 05:43:45, sof wrote: > On 2015/08/25 05:37:38, haraken wrote: > > On 2015/08/25 05:30:44, sof wrote: > > > Could someone explain why these race bugs are surfacing now/recently? > > > > I asked the same question offline. ssid's guess is that ssid's change > increased > > the overhead of TypenameStringTrait<T>::get() and made the threading race more > > likely. > > I haven't looked into details, but it cannot be TypenameStringTrait<T>::get > instead? TypenameStringTrait<T>::get calls WTF::extractTypeNameFromFunctionName(WTF::extractNameFunction<T>()), which can be heavy.
On 2015/08/25 05:46:28, haraken wrote: > On 2015/08/25 05:43:45, sof wrote: > > On 2015/08/25 05:37:38, haraken wrote: > > > On 2015/08/25 05:30:44, sof wrote: > > > > Could someone explain why these race bugs are surfacing now/recently? > > > > > > I asked the same question offline. ssid's guess is that ssid's change > > increased > > > the overhead of TypenameStringTrait<T>::get() and made the threading race > more > > > likely. > > > > I haven't looked into details, but it cannot be TypenameStringTrait<T>::get > > instead? > > TypenameStringTrait<T>::get calls > WTF::extractTypeNameFromFunctionName(WTF::extractNameFunction<T>()), which can > be heavy. Which is presumably why it is already being cached (and can continue to be.) Adding overhead for a non-user facing feature (tracing) here seems sub-optimal; which I think it is doing by having a non-static function call in a static variable + now adding an atomic static initialization. We've been trying to make the initialization of the GCInfoTable atomic only.
On 2015/08/25 06:35:15, sof wrote: > On 2015/08/25 05:46:28, haraken wrote: > > On 2015/08/25 05:43:45, sof wrote: > > > On 2015/08/25 05:37:38, haraken wrote: > > > > On 2015/08/25 05:30:44, sof wrote: > > > > > Could someone explain why these race bugs are surfacing now/recently? > > > > > > > > I asked the same question offline. ssid's guess is that ssid's change > > > increased > > > > the overhead of TypenameStringTrait<T>::get() and made the threading race > > more > > > > likely. > > > > > > I haven't looked into details, but it cannot be TypenameStringTrait<T>::get > > > instead? > > > > TypenameStringTrait<T>::get calls > > WTF::extractTypeNameFromFunctionName(WTF::extractNameFunction<T>()), which can > > be heavy. > > Which is presumably why it is already being cached (and can continue to be.) > > Adding overhead for a non-user facing feature (tracing) here seems sub-optimal; > which I think it is doing by having a non-static function call in a static > variable + now adding an atomic static initialization. We've been trying to make > the initialization of the GCInfoTable atomic only. I agree, I should not be adding overhead just for tracing. I first made it TypenameStringTrait<T>::get. But the issue was that the static strings were getting destructed at the exit. To hold reference to the strings, I made it initialize. The call to TypenameStringTrait<T>::get() could be heavy, but it gets called only the first time the type is initialized. So, I thought it is fine. Also, the atomic init was already added for the string initialization, I just moved it in this CL. What I can do to make it more efficient: Use TypenameStringTrait<T>::get instead of calling the method. (something like https://codereview.chromium.org/1287963002/). Not use DEFINE_STATIC_LOCAL in TypenameStringTrait<T>::get since the string references are destructed. But define a static String* instead to leak the strings. None of this needs to be atomic, since the get is already called in thread safe way. But the issue is that the static definition of GCInfo struct there itself seems racy. I did not go for the option i just mentioned because this issue should be fixed as well. Is the plan not to fix this and assume that this race is very rare?
On 2015/08/25 09:34:51, ssid wrote: > On 2015/08/25 06:35:15, sof wrote: > > On 2015/08/25 05:46:28, haraken wrote: > > > On 2015/08/25 05:43:45, sof wrote: > > > > On 2015/08/25 05:37:38, haraken wrote: > > > > > On 2015/08/25 05:30:44, sof wrote: > > > > > > Could someone explain why these race bugs are surfacing now/recently? > > > > > > > > > > I asked the same question offline. ssid's guess is that ssid's change > > > > increased > > > > > the overhead of TypenameStringTrait<T>::get() and made the threading > race > > > more > > > > > likely. > > > > > > > > I haven't looked into details, but it cannot be > TypenameStringTrait<T>::get > > > > instead? > > > > > > TypenameStringTrait<T>::get calls > > > WTF::extractTypeNameFromFunctionName(WTF::extractNameFunction<T>()), which > can > > > be heavy. > > > > Which is presumably why it is already being cached (and can continue to be.) > > > > Adding overhead for a non-user facing feature (tracing) here seems > sub-optimal; > > which I think it is doing by having a non-static function call in a static > > variable + now adding an atomic static initialization. We've been trying to > make > > the initialization of the GCInfoTable atomic only. > > > I agree, I should not be adding overhead just for tracing. I first made it > TypenameStringTrait<T>::get. But the issue was that the static strings were > getting destructed at the exit. To hold reference to the strings, I made it > initialize. > The call to TypenameStringTrait<T>::get() could be heavy, but it gets called > only the first time the type is initialized. So, I thought it is fine. Also, the > atomic init was already added for the string initialization, I just moved it in > this CL. > I could well be overlooking some use, but it seems to me that the className() is only used to generate dump names ThreadState::takeSnapshot(). Couldn't the temporary strings be constructed there each time around -- extracting the substring from extractTypeNameFromFunctionName() is not that expensive, relatively speaking? Or has that been tried and found to be wanting wrt perf, leading to the AtomicString + singleton rep. that's now used? (Another alternative is to keep class names in table separate from GCInfos, I suppose.)
> I could well be overlooking some use, but it seems to me that the className() is > only used to generate dump names ThreadState::takeSnapshot(). Couldn't the > temporary strings be constructed there each time around -- extracting the > substring from extractTypeNameFromFunctionName() is not that expensive, > relatively speaking? Or has that been tried and found to be wanting wrt perf, > leading to the AtomicString + singleton rep. that's now used? > > (Another alternative is to keep class names in table separate from GCInfos, I > suppose.) Yes, that could work for tracing. yes it is only being used in takeSnapshot. Are you suggesting that I leave the GCInfo as it is and not make it thread safe?
On 2015/08/25 15:36:17, ssid wrote: > > I could well be overlooking some use, but it seems to me that the className() > is > > only used to generate dump names ThreadState::takeSnapshot(). Couldn't the > > temporary strings be constructed there each time around -- extracting the > > substring from extractTypeNameFromFunctionName() is not that expensive, > > relatively speaking? Or has that been tried and found to be wanting wrt perf, > > leading to the AtomicString + singleton rep. that's now used? > > > > (Another alternative is to keep class names in table separate from GCInfos, I > > suppose.) > > Yes, that could work for tracing. yes it is only being used in takeSnapshot. > Are you suggesting that I leave the GCInfo as it is and not make it thread safe? The initializer for the GCInfo static would then be just static data again, so if we have no known data race bugs against the GCInfo table locking protocol (afaik), then that seems preferable. I'm not going to block this CL though, as you've gotten the approval.
On 2015/08/25 15:52:14, sof wrote: > On 2015/08/25 15:36:17, ssid wrote: > > > I could well be overlooking some use, but it seems to me that the > className() > > is > > > only used to generate dump names ThreadState::takeSnapshot(). Couldn't the > > > temporary strings be constructed there each time around -- extracting the > > > substring from extractTypeNameFromFunctionName() is not that expensive, > > > relatively speaking? Or has that been tried and found to be wanting wrt > perf, > > > leading to the AtomicString + singleton rep. that's now used? > > > > > > (Another alternative is to keep class names in table separate from GCInfos, > I > > > suppose.) > > > > Yes, that could work for tracing. yes it is only being used in takeSnapshot. > > Are you suggesting that I leave the GCInfo as it is and not make it thread > safe? > > > The initializer for the GCInfo static would then be just static data again, so > if we have no known data race bugs against the GCInfo table locking protocol > (afaik), then that seems preferable. > > I'm not going to block this CL though, as you've gotten the approval. Yeah I have looked through the history of the data races. It has never happened before adding the className param. I would prefer to not add too in that case. I just thought it it right thing to make it atomic so made this change. I will rework this CL without atomic on GCInfo. Thanks for your feedback.
On 2015/08/25 15:56:44, ssid wrote: > On 2015/08/25 15:52:14, sof wrote: > > On 2015/08/25 15:36:17, ssid wrote: > > > > I could well be overlooking some use, but it seems to me that the > > className() > > > is > > > > only used to generate dump names ThreadState::takeSnapshot(). Couldn't the > > > > temporary strings be constructed there each time around -- extracting the > > > > substring from extractTypeNameFromFunctionName() is not that expensive, > > > > relatively speaking? Or has that been tried and found to be wanting wrt > > perf, > > > > leading to the AtomicString + singleton rep. that's now used? > > > > > > > > (Another alternative is to keep class names in table separate from > GCInfos, > > I > > > > suppose.) > > > > > > Yes, that could work for tracing. yes it is only being used in takeSnapshot. > > > Are you suggesting that I leave the GCInfo as it is and not make it thread > > safe? > > > > > > The initializer for the GCInfo static would then be just static data again, so > > if we have no known data race bugs against the GCInfo table locking protocol > > (afaik), then that seems preferable. > > > > I'm not going to block this CL though, as you've gotten the approval. > > Yeah I have looked through the history of the data races. It has never happened > before adding the className param. I would prefer to not add too in that case. > I just thought it it right thing to make it atomic so made this change. I will > rework this CL without atomic on GCInfo. > Thanks for your feedback. Let's go with sof's proposal. That sounds much better :)
ssid@chromium.org changed reviewers: + haraken@chromium.org
haraken@ PTAL, thanks.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312963002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312963002/20001
LGTM, but I want to have sof@ confirm.
lgtm also. This will break ENABLE(GC_PROFILING) usage a bit, but we can handle that separately.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312963002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312963002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201223 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
