Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2366283003:
Remove runtime zone. (Closed)
DescriptionRemove the runtime zone. The runtime zone is ugly
because ownership over it is not obviously clear
and leads to errors.
Committed: https://crrev.com/506c9bcd46ffddab32fd16d90e7ec8f42a70ddf4
Cr-Commit-Position: refs/heads/master@{#40024}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Keep list storage memory #Patch Set 3 : Reaction to comments #
Total comments: 5
Patch Set 4 : Reaction to comments #
Total comments: 2
Patch Set 5 : Reaction to comments #Patch Set 6 : Reaction to comments #
Total comments: 1
Messages
Total messages: 54 (36 generated)
The CQ bit was checked by heimbuef@google.com 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.
heimbuef@google.com changed reviewers: + erik cory@chromium.org
PTAL
heimbuef@google.com changed reviewers: + jakob.gruber@gmail.com
heimbuef@google.com changed reviewers: + erikcorry@chromium.org, jgruber@chromium.org - erik cory@chromium.org, jakob.gruber@gmail.com
A couple of comments: https://codereview.chromium.org/2366283003/diff/1/src/isolate.h File src/isolate.h (right): https://codereview.chromium.org/2366283003/diff/1/src/isolate.h#newcode1345 src/isolate.h:1345: List<int> regex_indices_; Nit: 'regexp' here and above, and please move this just below regexp_stack. https://codereview.chromium.org/2366283003/diff/1/src/runtime/runtime-regexp.cc File src/runtime/runtime-regexp.cc (right): https://codereview.chromium.org/2366283003/diff/1/src/runtime/runtime-regexp.... src/runtime/runtime-regexp.cc:391: indices->Clear(); As discussed offline, this deletes indices.data_ just to reallocate it once Add() is called later on. Also, we should probably check whether indices is very large at the end of this function, and free it if that's the case. https://codereview.chromium.org/2366283003/diff/1/src/runtime/runtime-regexp.... src/runtime/runtime-regexp.cc:710: List<int> indices(initial_capacity); Couldn't you reuse regexp_list here as well?
On 2016/09/30 at 09:46:24, jgruber wrote: > A couple of comments: > > https://codereview.chromium.org/2366283003/diff/1/src/isolate.h > File src/isolate.h (right): > > https://codereview.chromium.org/2366283003/diff/1/src/isolate.h#newcode1345 > src/isolate.h:1345: List<int> regex_indices_; > Nit: 'regexp' here and above, and please move this just below regexp_stack. > > https://codereview.chromium.org/2366283003/diff/1/src/runtime/runtime-regexp.cc > File src/runtime/runtime-regexp.cc (right): > > https://codereview.chromium.org/2366283003/diff/1/src/runtime/runtime-regexp.... > src/runtime/runtime-regexp.cc:391: indices->Clear(); > As discussed offline, this deletes indices.data_ just to reallocate it once Add() is called later on. > > Also, we should probably check whether indices is very large at the end of this function, and free it if that's the case. > > https://codereview.chromium.org/2366283003/diff/1/src/runtime/runtime-regexp.... > src/runtime/runtime-regexp.cc:710: List<int> indices(initial_capacity); > Couldn't you reuse regexp_list here as well? All Done. PTAL.
Looking good. A few more comments below, and please be a bit more descriptive in the CL description. Why is the runtime zone being removed? How is performance affected? https://codereview.chromium.org/2366283003/diff/40001/src/isolate.h File src/isolate.h (right): https://codereview.chromium.org/2366283003/diff/40001/src/isolate.h#newcode928 src/isolate.h:928: List<int>* regex_indices() { return ®ex_indices_; } Nit: This could be const. And please rename to regexp_indices. https://codereview.chromium.org/2366283003/diff/40001/src/isolate.h#newcode1341 src/isolate.h:1341: List<int> regex_indices_; Please rename to regexp_indices_ https://codereview.chromium.org/2366283003/diff/40001/src/list-inl.h File src/list-inl.h (right): https://codereview.chromium.org/2366283003/diff/40001/src/list-inl.h#newcode144 src/list-inl.h:144: void List<T, P>::Clear() { Is this left over from local experiments? Clear should preserve old semantics. https://codereview.chromium.org/2366283003/diff/40001/src/runtime/runtime-reg... File src/runtime/runtime-regexp.cc (right): https://codereview.chromium.org/2366283003/diff/40001/src/runtime/runtime-reg... src/runtime/runtime-regexp.cc:458: if (indices->capacity() > (1 << 13)) { Please use a constant for this, e.g. static const int kMaxRegExpIndicesCapacity. Is 2^13 identical to current behavior when using zone-allocated lists? https://codereview.chromium.org/2366283003/diff/40001/src/runtime/runtime-reg... src/runtime/runtime-regexp.cc:712: List<int>* indices = isolate->regex_indices(); We should probably also handle very large regexp_indices before returning from this function as above.
Description was changed from ========== Remove runtime zone. BUG= ========== to ========== Remove the runtime zone. The runtime zone is ugly because ownership over it is not obviously clear and leads to errors. ==========
lgtm
The CQ bit was checked by heimbuef@google.com 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...
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by heimbuef@google.com 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.
The CQ bit was checked by heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2366283003/#ps80001 (title: "Reaction to comments")
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/25568)
The CQ bit was checked by heimbuef@google.com
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/25575)
heimbuef@google.com changed reviewers: + ahaas@chromium.org - erikcorry@chromium.org
lgtm after comments are addressed https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... File src/runtime/runtime-regexp.cc (right): https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... src/runtime/runtime-regexp.cc:391: indices->Rewind(0); Could you write a wrapper function for these two lines? You use the same lines below. https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... src/runtime/runtime-regexp.cc:458: if (indices->capacity() > Isolate::kMaxRegexpIndicesSize) { What do you check here? Could you add a comment, or could you write a wrapper function for it?
On 2016/10/05 at 08:32:13, ahaas wrote: > lgtm after comments are addressed > > https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... > File src/runtime/runtime-regexp.cc (right): > > https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... > src/runtime/runtime-regexp.cc:391: indices->Rewind(0); > Could you write a wrapper function for these two lines? You use the same lines below. I don't think I can. The variable declaration is still needed, and what's then left for the helper function to do? > > https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... > src/runtime/runtime-regexp.cc:458: if (indices->capacity() > Isolate::kMaxRegexpIndicesSize) { > What do you check here? Could you add a comment, or could you write a wrapper function for it? Done.
The CQ bit was checked by heimbuef@google.com 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...
On 2016/10/05 at 08:45:22, heimbuef wrote: > On 2016/10/05 at 08:32:13, ahaas wrote: > > lgtm after comments are addressed > > > > https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... > > File src/runtime/runtime-regexp.cc (right): > > > > https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... > > src/runtime/runtime-regexp.cc:391: indices->Rewind(0); > > Could you write a wrapper function for these two lines? You use the same lines below. > > I don't think I can. The variable declaration is still needed, and what's then left for the helper function to do? > The helper function would combine isolate->regexp_indices() and indices->Rewind(0). In my understanding the purpose of these two calls is to acquire an empty list. However, this purpose is not obvious when you read the two lines. First I thought that you should just add a comment, but actually I think that a helper function would be a better approach. > > > > https://codereview.chromium.org/2366283003/diff/80001/src/runtime/runtime-reg... > > src/runtime/runtime-regexp.cc:458: if (indices->capacity() > Isolate::kMaxRegexpIndicesSize) { > > What do you check here? Could you add a comment, or could you write a wrapper function for it? > > Done.
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 heimbuef@google.com 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...
lgtm with nit https://codereview.chromium.org/2366283003/diff/120001/src/runtime/runtime-re... File src/runtime/runtime-regexp.cc (right): https://codereview.chromium.org/2366283003/diff/120001/src/runtime/runtime-re... src/runtime/runtime-regexp.cc:384: static List<int>* GetRewindedRegexpIndicesList(Isolate* isolate) { putting a function into an anonymous namespace is the same as making the function static, i.e. no need to use static here.
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 heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2366283003/#ps120001 (title: "Reaction to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove the runtime zone. The runtime zone is ugly because ownership over it is not obviously clear and leads to errors. ========== to ========== Remove the runtime zone. The runtime zone is ugly because ownership over it is not obviously clear and leads to errors. Committed: https://crrev.com/506c9bcd46ffddab32fd16d90e7ec8f42a70ddf4 Cr-Commit-Position: refs/heads/master@{#40024} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/506c9bcd46ffddab32fd16d90e7ec8f42a70ddf4 Cr-Commit-Position: refs/heads/master@{#40024}
The CQ bit was checked by heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org, jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2366283003/#ps140001 (title: "Remove unnecessary statics")
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_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
Patchset #7 (id:140001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
