Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887193002:
Create a thread safe version of StatsCounters and use. (Closed)
DescriptionCreate a thread safe version of StatsCounters and use.
Creates a new class StatsCounterThreadSafe to be used by counters that
can be updated when compiling/decoding etc. are done using workers.
Does this by using a mutex on all opreations.
Also updates the StatsCounterThreadSafe constructor to force counter
initialization, as well as method Reset(). In addition, whenever the
method StatsTable::SetCounterFunction() is called (from the main
thread), it forces counter initialization for all thread safe stats
counters.
BUG=v8:6361
Review-Url: https://codereview.chromium.org/2887193002
Cr-Commit-Position: refs/heads/master@{#45526}
Committed: https://chromium.googlesource.com/v8/v8/+/fbbc0ff24328fe966cc9538976f693601f44e5d1
Patch Set 1 #Patch Set 2 : Clean up nits. #Patch Set 3 : Clean up code. #
Total comments: 2
Patch Set 4 : Make Counters a shared pointer, to allow workers to hold on to them. #Patch Set 5 : Clean up nits. #
Total comments: 6
Patch Set 6 : Factor out base class StatsCounterBase #
Total comments: 2
Patch Set 7 : Replace NULL with nullptr. #
Total comments: 10
Patch Set 8 : fix nits of mtrofin #
Total comments: 12
Messages
Total messages: 41 (22 generated)
The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Create a thread safe version of StatsCounters and use. Creates a new class StatsCounterThreadSafe to be used by counters that can be updated when compiling/decoding etc. are done using workers. Does this by using a mutex on all opreations. Also updates the StatsCounterThreadSafe constructor to force counter initialization, as well as method Reset(). In addition, whenever the method StatsTable::SetCounterFunction() is called (from the main thread), it forces counter initialization for all thread safe stats counters. BUG=v8:6361 ========== to ========== Create a thread safe version of StatsCounters and use. Creates a new class StatsCounterThreadSafe to be used by counters that can be updated when compiling/decoding etc. are done using workers. Does this by using a mutex on all opreations. Also updates the StatsCounterThreadSafe constructor to force counter initialization, as well as method Reset(). In addition, whenever the method StatsTable::SetCounterFunction() is called (from the main thread), it forces counter initialization for all thread safe stats counters. BUG=v8:6361 ==========
kschimpf@chromium.org changed reviewers: + bradnelson@chromium.org, jochen@chromium.org, mtrofin@chromium.org
Please review. Thanks.
https://codereview.chromium.org/2887193002/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2887193002/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:83: } that doesn't look safe - isolate still can't be deref'd on a background thread. Instead, you'd need to pass the Counters* point around
https://codereview.chromium.org/2887193002/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2887193002/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:83: } On 2017/05/18 20:32:45, jochen wrote: > that doesn't look safe - isolate still can't be deref'd on a background thread. > Instead, you'd need to pass the Counters* point around Would I create this Counters* value from the isolate->counter(), and save it as a field of class AsyncCompileJob (in the same file), and then use that value in the worker? Or, must I create a separate Counters object need to be created for the instance of AsyncCompileJob?
On 2017/05/18 at 21:05:24, kschimpf wrote: > https://codereview.chromium.org/2887193002/diff/40001/src/wasm/wasm-module.cc > File src/wasm/wasm-module.cc (left): > > https://codereview.chromium.org/2887193002/diff/40001/src/wasm/wasm-module.cc... > src/wasm/wasm-module.cc:83: } > On 2017/05/18 20:32:45, jochen wrote: > > that doesn't look safe - isolate still can't be deref'd on a background thread. > > Instead, you'd need to pass the Counters* point around > > Would I create this Counters* value from the isolate->counter(), and save it as a field of class AsyncCompileJob (in the same file), and then use that value in the worker? Or, must I create a separate Counters object need to be created for the instance of AsyncCompileJob? Just storing the result of isolate->counters () while on the main thread should work, yes
After looking at this some more, using isolate->counters() isn't any safer. All that method does is return a pointer field, and that pointer field is deallocated in the destructor of an isolate. If I changed the cached pointer to a shared_ptr, that would guarantee the safety.
The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ready for another review. Thanks.
it's a bit sad that we now have to take a mutex all the time even if the counters aren't used (and they aren't used in chrome). Maybe the threadsafe version could locally remember that there is no counter callback instead of getting a lock all the time? https://codereview.chromium.org/2887193002/diff/80001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2887193002/diff/80001/src/counters.h#newcode109 src/counters.h:109: void Set(int value) { Set(GetPtr(), value); } having that all public looks odd. Could the thread safe version inherit from StatsCounter? Maybe there should be a StatsCounterBase or something that both inherit from? https://codereview.chromium.org/2887193002/diff/80001/src/counters.h#newcode182 src/counters.h:182: explicit StatsCounterThreadSafe(Isolate* isolate, const char* name); nit. no explicit https://codereview.chromium.org/2887193002/diff/80001/src/counters.h#newcode1287 src/counters.h:1287: // clang-format off hum, why?
The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Refactored out base class StatsCounterBase. Also added check in the thread safe version to do nothing if no corresponding counter exists. https://codereview.chromium.org/2887193002/diff/80001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2887193002/diff/80001/src/counters.h#newcode109 src/counters.h:109: void Set(int value) { Set(GetPtr(), value); } On 2017/05/23 11:29:20, jochen wrote: > having that all public looks odd. Could the thread safe version inherit from > StatsCounter? Maybe there should be a StatsCounterBase or something that both > inherit from? I don't understand this comment. The thread safe version inherits from this class (StatsCounter). I did not widen the public API. The public non-pointer version calls a protected pointer version. However, I also see the argument for possibly creating a base class. Changing to use a common base class. https://codereview.chromium.org/2887193002/diff/80001/src/counters.h#newcode182 src/counters.h:182: explicit StatsCounterThreadSafe(Isolate* isolate, const char* name); On 2017/05/23 11:29:21, jochen wrote: > nit. no explicit Done. https://codereview.chromium.org/2887193002/diff/80001/src/counters.h#newcode1287 src/counters.h:1287: // clang-format off On 2017/05/23 11:29:20, jochen wrote: > hum, why? Because clang-format changes the following Defines/definitions into a highly nested unreadable mess. I do not know why this is (whether it is a change in clang-format, or this code was originally committed without clang formatting).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2887193002/diff/100001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2887193002/diff/100001/src/isolate.cc#newcode... src/isolate.cc:2640: if (counters_ != NULL) return false; nit. nullptr
The CQ bit was checked by kschimpf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2887193002/#ps120001 (title: "Replace NULL with nullptr.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/41807)
https://codereview.chromium.org/2887193002/diff/100001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2887193002/diff/100001/src/isolate.cc#newcode... src/isolate.cc:2640: if (counters_ != NULL) return false; On 2017/05/24 19:41:07, jochen wrote: > nit. nullptr Done.
LGTM with nits. https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:59: static void RecordStats(Isolate* isolate, Code* code, Counters* counters) { let's remove the isolate param here... https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:64: static void RecordStats(Isolate* isolate, Handle<FixedArray> functions, ...and here. https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:914: void RecordLazyCodeStats(Isolate* isolate, Code* code, Counters* counters_) { Please remove isolate param. Please rename counters_ -> counters https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:3339: Counters* counters_) { please remove _ in counters_ https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.h... src/wasm/wasm-module.h:505: Counters* counters_); Please remove "_" here
https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:59: static void RecordStats(Isolate* isolate, Code* code, Counters* counters) { On 2017/05/24 20:37:20, Mircea Trofin wrote: > let's remove the isolate param here... Done. https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:64: static void RecordStats(Isolate* isolate, Handle<FixedArray> functions, On 2017/05/24 20:37:20, Mircea Trofin wrote: > ...and here. Done. https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:914: void RecordLazyCodeStats(Isolate* isolate, Code* code, Counters* counters_) { On 2017/05/24 20:37:20, Mircea Trofin wrote: > Please remove isolate param. > > Please rename counters_ -> counters Done. https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:3339: Counters* counters_) { On 2017/05/24 20:37:20, Mircea Trofin wrote: > please remove _ in counters_ Done. https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2887193002/diff/120001/src/wasm/wasm-module.h... src/wasm/wasm-module.h:505: Counters* counters_); On 2017/05/24 20:37:20, Mircea Trofin wrote: > Please remove "_" here Done.
The CQ bit was checked by kschimpf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2887193002/#ps140001 (title: "fix nits of mtrofin")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495658965828330,
"parent_rev": "709c906a9200b0a1401b58261f311de95707adc0", "commit_rev":
"fbbc0ff24328fe966cc9538976f693601f44e5d1"}
Message was sent while issue was closed.
Description was changed from ========== Create a thread safe version of StatsCounters and use. Creates a new class StatsCounterThreadSafe to be used by counters that can be updated when compiling/decoding etc. are done using workers. Does this by using a mutex on all opreations. Also updates the StatsCounterThreadSafe constructor to force counter initialization, as well as method Reset(). In addition, whenever the method StatsTable::SetCounterFunction() is called (from the main thread), it forces counter initialization for all thread safe stats counters. BUG=v8:6361 ========== to ========== Create a thread safe version of StatsCounters and use. Creates a new class StatsCounterThreadSafe to be used by counters that can be updated when compiling/decoding etc. are done using workers. Does this by using a mutex on all opreations. Also updates the StatsCounterThreadSafe constructor to force counter initialization, as well as method Reset(). In addition, whenever the method StatsTable::SetCounterFunction() is called (from the main thread), it forces counter initialization for all thread safe stats counters. BUG=v8:6361 Review-Url: https://codereview.chromium.org/2887193002 Cr-Commit-Position: refs/heads/master@{#45526} Committed: https://chromium.googlesource.com/v8/v8/+/fbbc0ff24328fe966cc9538976f693601f4... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/fbbc0ff24328fe966cc9538976f693601f4...
Message was sent while issue was closed.
clemensh@chromium.org changed reviewers: + clemensh@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2887193002/diff/140001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2887193002/diff/140001/src/counters.h#newcode107 src/counters.h:107: void IncrementLoc(int* loc) { (*loc)++; } Is there any implementation where the semantics of {IncrementLoc(loc)} is different from {IncrementLoc(loc, 1)}? If no, we should just set a default value of 1 for the {int value} parameter. (Also on all other occurences of this pattern) https://codereview.chromium.org/2887193002/diff/140001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2887193002/diff/140001/src/isolate.cc#newcode... src/isolate.cc:2640: if (counters_ != nullptr) return false; This pattern is not safe in general. A thread observing that {counters_ != nullptr} is not guaranteed to also observe all other updates performed inside the locked scope below. This would at least require atomic updates of the {counters_} field, or explicit memory barries. Overall, I don't see why we need the mutex here in the first place. Is it allowed to call this function from a background task? https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:357: Counters* counters_; Why is this field needed? {counters_shared_.get()} should be equally good. It's just reading a field from our own shared_ptr instance, so no potential race conditions here. https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:1561: Counters* counters_; Same here.
Message was sent while issue was closed.
https://codereview.chromium.org/2887193002/diff/140001/src/counters.h File src/counters.h (left): https://codereview.chromium.org/2887193002/diff/140001/src/counters.h#oldcode121 src/counters.h:121: void Increment(int value) { Here was the previous definition of increment. https://codereview.chromium.org/2887193002/diff/140001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2887193002/diff/140001/src/counters.h#newcode107 src/counters.h:107: void IncrementLoc(int* loc) { (*loc)++; } On 2017/06/02 13:08:49, Clemens Hammacher wrote: > Is there any implementation where the semantics of {IncrementLoc(loc)} is > different from {IncrementLoc(loc, 1)}? If no, we should just set a default value > of 1 for the {int value} parameter. > (Also on all other occurences of this pattern) I did this to follow the same pattern as the code it replaced. However, I'll look to see if it is unnecessary. https://codereview.chromium.org/2887193002/diff/140001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2887193002/diff/140001/src/isolate.cc#newcode... src/isolate.cc:2640: if (counters_ != nullptr) return false; On 2017/06/02 13:08:49, Clemens Hammacher wrote: > This pattern is not safe in general. A thread observing that {counters_ != > nullptr} is not guaranteed to also observe all other updates performed inside > the locked scope below. This would at least require atomic updates of the > {counters_} field, or explicit memory barries. > > Overall, I don't see why we need the mutex here in the first place. Is it > allowed to call this function from a background task? I was probably too concerned about this, since the way code was previously written, it could be called on background threads. However, I agree that we probably could have presumed this. Now that the StatsTable of an Isolate has been moved to a field in the Counters class, we can make stronger assumptions (by not making this method public). Will fix as part of CL https://codereview.chromium.org/2918703002 https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:357: Counters* counters_; On 2017/06/02 13:08:50, Clemens Hammacher wrote: > Why is this field needed? {counters_shared_.get()} should be equally good. It's > just reading a field from our own shared_ptr instance, so no potential race > conditions here. I was confusing the assignment operator with the get() method, where there is a LOT of inline code. You appear to be correct that I should have used the get() method. Fixing in separate CL.
Message was sent while issue was closed.
https://codereview.chromium.org/2887193002/diff/140001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2887193002/diff/140001/src/counters.h#newcode107 src/counters.h:107: void IncrementLoc(int* loc) { (*loc)++; } On 2017/06/02 15:01:02, kschimpf wrote: > On 2017/06/02 13:08:49, Clemens Hammacher wrote: > > Is there any implementation where the semantics of {IncrementLoc(loc)} is > > different from {IncrementLoc(loc, 1)}? If no, we should just set a default > value > > of 1 for the {int value} parameter. > > (Also on all other occurences of this pattern) > > I did this to follow the same pattern as the code it replaced. However, I'll > look to see if it is unnecessary. There are calls on "size" counters that increment counts by the size value. Hence they are needed. https://codereview.chromium.org/2887193002/diff/140001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2887193002/diff/140001/src/isolate.cc#newcode... src/isolate.cc:2640: if (counters_ != nullptr) return false; On 2017/06/02 15:01:02, kschimpf wrote: > On 2017/06/02 13:08:49, Clemens Hammacher wrote: > > This pattern is not safe in general. A thread observing that {counters_ != > > nullptr} is not guaranteed to also observe all other updates performed inside > > the locked scope below. This would at least require atomic updates of the > > {counters_} field, or explicit memory barries. > > > > Overall, I don't see why we need the mutex here in the first place. Is it > > allowed to call this function from a background task? > > I was probably too concerned about this, since the way code was previously > written, it could be called on background threads. However, I agree that we > probably could have presumed this. Now that the StatsTable of an Isolate has > been moved to a field in the Counters class, we can make stronger assumptions > (by not making this method public). Will fix as part of CL > https://codereview.chromium.org/2918703002 Fixed in cl https://codereview.chromium.org/2919953003 https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:357: Counters* counters_; On 2017/06/02 15:01:02, kschimpf wrote: > On 2017/06/02 13:08:50, Clemens Hammacher wrote: > > Why is this field needed? {counters_shared_.get()} should be equally good. > It's > > just reading a field from our own shared_ptr instance, so no potential race > > conditions here. > > I was confusing the assignment operator with the get() method, where there is a > LOT of inline code. You appear to be correct that I should have used the get() > method. Fixing in separate CL. Fixed in CL https://codereview.chromium.org/2919953003 https://codereview.chromium.org/2887193002/diff/140001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:1561: Counters* counters_; On 2017/06/02 13:08:50, Clemens Hammacher wrote: > Same here. Fixed in CL https://codereview.chromium.org/2919953003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
