Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
138953018:
Use NoBarrier_Load and NoBarrier_Store in FreeListCategory::Concatenate. (Closed)
|
Created:
6 years, 10 months ago by Hannes Payer (out of office) Modified:
6 years, 10 months ago Reviewers:
Jarin CC:
v8-dev Visibility:
Public. |
DescriptionUse NoBarrier_Load and NoBarrier_Store in FreeListCategory::Concatenate.
BUG=
R=jarin@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=19355
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/138953018/diff/1/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/138953018/diff/1/src/spaces.cc#newcode2085 src/spaces.cc:2085: if (NoBarrier_Load(reinterpret_cast<AtomicWord*>(&category->top_)) != 0) { You should change the FreeListCategory::top_ field to be AtomicWord rather than casting.
https://codereview.chromium.org/138953018/diff/1/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/138953018/diff/1/src/spaces.cc#newcode2085 src/spaces.cc:2085: if (NoBarrier_Load(reinterpret_cast<AtomicWord*>(&category->top_)) != 0) { On 2014/02/12 14:09:02, jarin wrote: > You should change the FreeListCategory::top_ field to be AtomicWord rather than > casting. Done.
I know that this is massively annoying, but AtomicWord that could participate in a data race should never be cast to a normal pointer (undefined behavior in C++11). Instead, you should always use the Store/Load function to access the atomic memory. Sadly, this might require some pretty awkward code - I suggested one solution in the comments, but you might come up with something prettier. Thanks for doing this, it is great that you are cleaning up our parallelism story. https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2085 src/spaces.cc:2085: if (NoBarrier_Load(&category->top_) != 0) { Use the top() accessor. https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2098 src/spaces.cc:2098: NoBarrier_Store(&top_, category->top_); Use the set_top() and top() accessors. https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2115 src/spaces.cc:2115: FreeListNode** n = GetTopAddress(); Replace with FreeListNode* t = top(); FreeListNode** n = &t; .... loop .... set_top(t); https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2134 src/spaces.cc:2134: FreeListNode** n = GetTopAddress(); Replace with the gymnastics with accessors (or unroll the loop once). https://codereview.chromium.org/138953018/diff/50001/src/spaces.h File src/spaces.h (right): https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1525 src/spaces.h:1525: return reinterpret_cast<FreeListNode**>(&top_); Unfortunately, this would not be legal by the C++11 spec - once a reference is AtomicWord, it should only be accessed via the (Release|Acquire|NoBarrier)_(Load|Store) functions. It should never be cast to a normal pointer. We will have to get rid of this method and use the normal accessors before and after the loop where the method is used (see the comments in spaces.cc). https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1529 src/spaces.h:1529: return reinterpret_cast<FreeListNode*>(top_); Please, use NoBarrier_Load here. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1533 src/spaces.h:1533: top_ = reinterpret_cast<AtomicWord>(top); NoBarrier_Store. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1547 src/spaces.h:1547: return top_ == 0; Use the accessor rather than the direct access. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1556 src/spaces.h:1556: AtomicWord top_; At the end of the line, add // FreeListNode*
https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2085 src/spaces.cc:2085: if (NoBarrier_Load(&category->top_) != 0) { On 2014/02/12 21:38:02, jarin wrote: > Use the top() accessor. Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2098 src/spaces.cc:2098: NoBarrier_Store(&top_, category->top_); On 2014/02/12 21:38:02, jarin wrote: > Use the set_top() and top() accessors. Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2115 src/spaces.cc:2115: FreeListNode** n = GetTopAddress(); On 2014/02/12 21:38:02, jarin wrote: > Replace with > > FreeListNode* t = top(); > FreeListNode** n = &t; > > .... loop .... > > set_top(t); Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2134 src/spaces.cc:2134: FreeListNode** n = GetTopAddress(); On 2014/02/12 21:38:02, jarin wrote: > Replace with the gymnastics with accessors (or unroll the loop once). Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h File src/spaces.h (right): https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1525 src/spaces.h:1525: return reinterpret_cast<FreeListNode**>(&top_); On 2014/02/12 21:38:02, jarin wrote: > Unfortunately, this would not be legal by the C++11 spec - once a reference is > AtomicWord, it should only be accessed via the > (Release|Acquire|NoBarrier)_(Load|Store) functions. It should never be cast to a > normal pointer. We will have to get rid of this method and use the normal > accessors before and after the loop where the method is used (see the comments > in spaces.cc). Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1529 src/spaces.h:1529: return reinterpret_cast<FreeListNode*>(top_); On 2014/02/12 21:38:02, jarin wrote: > Please, use NoBarrier_Load here. Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1533 src/spaces.h:1533: top_ = reinterpret_cast<AtomicWord>(top); On 2014/02/12 21:38:02, jarin wrote: > NoBarrier_Store. Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1547 src/spaces.h:1547: return top_ == 0; On 2014/02/12 21:38:02, jarin wrote: > Use the accessor rather than the direct access. Done. https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1556 src/spaces.h:1556: AtomicWord top_; On 2014/02/12 21:38:02, jarin wrote: > At the end of the line, add // FreeListNode* Done.
On 2014/02/13 12:15:51, Hannes Payer wrote: > https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc > File src/spaces.cc (right): > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2085 > src/spaces.cc:2085: if (NoBarrier_Load(&category->top_) != 0) { > On 2014/02/12 21:38:02, jarin wrote: > > Use the top() accessor. > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2098 > src/spaces.cc:2098: NoBarrier_Store(&top_, category->top_); > On 2014/02/12 21:38:02, jarin wrote: > > Use the set_top() and top() accessors. > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2115 > src/spaces.cc:2115: FreeListNode** n = GetTopAddress(); > On 2014/02/12 21:38:02, jarin wrote: > > Replace with > > > > FreeListNode* t = top(); > > FreeListNode** n = &t; > > > > .... loop .... > > > > set_top(t); > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.cc#newcode2134 > src/spaces.cc:2134: FreeListNode** n = GetTopAddress(); > On 2014/02/12 21:38:02, jarin wrote: > > Replace with the gymnastics with accessors (or unroll the loop once). > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.h > File src/spaces.h (right): > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1525 > src/spaces.h:1525: return reinterpret_cast<FreeListNode**>(&top_); > On 2014/02/12 21:38:02, jarin wrote: > > Unfortunately, this would not be legal by the C++11 spec - once a reference is > > AtomicWord, it should only be accessed via the > > (Release|Acquire|NoBarrier)_(Load|Store) functions. It should never be cast to > a > > normal pointer. We will have to get rid of this method and use the normal > > accessors before and after the loop where the method is used (see the comments > > in spaces.cc). > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1529 > src/spaces.h:1529: return reinterpret_cast<FreeListNode*>(top_); > On 2014/02/12 21:38:02, jarin wrote: > > Please, use NoBarrier_Load here. > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1533 > src/spaces.h:1533: top_ = reinterpret_cast<AtomicWord>(top); > On 2014/02/12 21:38:02, jarin wrote: > > NoBarrier_Store. > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1547 > src/spaces.h:1547: return top_ == 0; > On 2014/02/12 21:38:02, jarin wrote: > > Use the accessor rather than the direct access. > > Done. > > https://codereview.chromium.org/138953018/diff/50001/src/spaces.h#newcode1556 > src/spaces.h:1556: AtomicWord top_; > On 2014/02/12 21:38:02, jarin wrote: > > At the end of the line, add // FreeListNode* > > Done. lgtm with a couple of questions.
Here are the questions. https://codereview.chromium.org/138953018/diff/110001/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/138953018/diff/110001/src/spaces.cc#newcode2131 src/spaces.cc:2131: FreeListNode** n = &t; Now I am wondering why we can't use t directly here (replace '*n' with 't' in the loop below). https://codereview.chromium.org/138953018/diff/110001/src/spaces.cc#newcode2314 src/spaces.cc:2314: huge_list_.set_top(top_node); This is a bit funny - which part of the loop reads huge_list_?
https://codereview.chromium.org/138953018/diff/110001/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/138953018/diff/110001/src/spaces.cc#newcode2131 src/spaces.cc:2131: FreeListNode** n = &t; On 2014/02/13 12:35:49, jarin wrote: > Now I am wondering why we can't use t directly here (replace '*n' with 't' in > the loop below). For sure we can, legacy code. We could have done that before as well. https://codereview.chromium.org/138953018/diff/110001/src/spaces.cc#newcode2314 src/spaces.cc:2314: huge_list_.set_top(top_node); On 2014/02/13 12:35:49, jarin wrote: > This is a bit funny - which part of the loop reads huge_list_? This store to top is not needed here since it is done outside the loop.
Message was sent while issue was closed.
Committed patchset #4 manually as r19355 (presubmit successful). |
