Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1994643002:
Simpler implementation of expression classifiers (Closed)
|
Created:
4 years, 7 months ago by nickie Modified:
4 years, 6 months ago Reviewers:
caitp (gmail) CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@nickie-1708193003-class-rev Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionSimpler implementation of expression classifiers
Just appending arrays, instead of cherry-picking elements.
This seems to be not only worse in memory, but also more
time consuming in CodeLoad.
This is a fork of https://codereview.chromium.org/1708193003/
I suggest NOT TO LAND this, in its present form.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Bug fix #Patch Set 3 : Bug fix #Patch Set 4 : Rebased #Patch Set 5 : Bug fix #Messages
Total messages: 26 (12 generated)
The CQ bit was checked by nikolaos@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/1994643002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994643002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...) v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/1994643002/diff/1/src/parsing/expression-clas... File src/parsing/expression-classifier.h (right): https://codereview.chromium.org/1994643002/diff/1/src/parsing/expression-clas... src/parsing/expression-classifier.h:381: reported_errors_->Rewind(reported_errors_end_); All this does is set the ring buffer's length_ to reported_errors_end_ --- what do we get out of this? Also, why do we have a `reported_errors_start_` concept? Does this need to be a ring buffer? It seems like we're doing a lot of start/end position management with no real purpose, unless I'm just missing it.
On 2016/05/18 15:23:21, caitp wrote: > https://codereview.chromium.org/1994643002/diff/1/src/parsing/expression-clas... > File src/parsing/expression-classifier.h (right): > > https://codereview.chromium.org/1994643002/diff/1/src/parsing/expression-clas... > src/parsing/expression-classifier.h:381: > reported_errors_->Rewind(reported_errors_end_); > All this does is set the ring buffer's length_ to reported_errors_end_ --- what > do we get out of this? Also, why do we have a `reported_errors_start_` concept? > Does this need to be a ring buffer? It seems like we're doing a lot of start/end > position management with no real purpose, unless I'm just missing it. It's not a ring buffer, it's more like a stack (only now there are seldom any pops). The 'start' and 'end' marks are important for being able to locate an error. Probably the 'start' mark can be eliminated, as the invariant is that the region of one classifier starts where the region of the "previous" one ends. I don't expect this to make much of a difference. I'm more worried by the fact that the bots failed on several tests --- I will have to see this. There must be a bug somewhere and it may invalidate the timing.
https://codereview.chromium.org/1994643002/diff/1/src/parsing/expression-clas... File src/parsing/expression-classifier.h (right): https://codereview.chromium.org/1994643002/diff/1/src/parsing/expression-clas... src/parsing/expression-classifier.h:380: if (errors & ~invalid_productions_) { In this if-then-else, the condition is reversed. Stupid bug...
The CQ bit was checked by nikolaos@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/1994643002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994643002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/5930) v8_linux_dbg_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by nikolaos@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/1994643002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994643002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_arm64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/16229)
The CQ bit was checked by nikolaos@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/1994643002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994643002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/6542) v8_linux_dbg_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by nikolaos@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/1994643002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994643002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/31 17:36:48, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. This is abandoned, in favour of https://codereview.chromium.org/1708193003/
Description was changed from ========== Simpler implementation of expression classifiers Just appending arrays, instead of cherry-picking elements. This seems to be not only worse in memory, but also more time consuming in CodeLoad. This is a fork of https://codereview.chromium.org/1708193003/ I suggest NOT TO LAND this, in its present form. ========== to ========== Simpler implementation of expression classifiers Just appending arrays, instead of cherry-picking elements. This seems to be not only worse in memory, but also more time consuming in CodeLoad. This is a fork of https://codereview.chromium.org/1708193003/ I suggest NOT TO LAND this, in its present form. ========== |
