Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2193813002:
Add parser finalization step (Closed)
|
Created:
4 years, 4 months ago by jochen (gone - plz use gerrit) Modified:
4 years, 4 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAdd parser finalization step
BUG=v8:5215
R=vogelheim@chromium.org,marja@chromium.org
Committed: https://crrev.com/f22ef1207d731f782dafb8872a1afe7401d323b5
Cr-Commit-Position: refs/heads/master@{#38203}
Patch Set 1 #
Total comments: 8
Patch Set 2 : updates #
Total comments: 2
Patch Set 3 : updates #
Messages
Total messages: 21 (11 generated)
ptal https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher-job.cc (right): https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher-job.cc:58: source_ = Handle<String>::cast(isolate_->global_handles()->Create(*source)); have to globalize the reference here, so it survives between function calls https://codereview.chromium.org/2193813002/diff/1/test/unittests/compiler-dis... File test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc (right): https://codereview.chromium.org/2193813002/diff/1/test/unittests/compiler-dis... test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:52: shared->set_end_position(source->length() - 1); is that correct? or should it be just source->length()
The CQ bit was checked by jochen@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...
lgtm, w/ some comments. I don't understand the parse result internalization logic, so it's probably best to wait for Marja's feedback. https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher-job.cc (left): https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher-job.cc:59: can_parse_on_background_thread_ = false; If the variable is no longer used, pls remove from class declaration. https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher-job.cc (right): https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher-job.cc:144: character_stream_.reset(); [general comment:] Since this class handles more & more resources, IMHO you should in the destructor either: - assert a proper terminal state (kFailed or kCompiled) - or enforce cleanup in the destructor. I prefer the former, since I guess the intention is that the dispatcher should run the state machine to its proper end (& hence the job-class can assume it will go through the proper life cycle). Loosely related: If the intention is that the dispatcher controls the lifecycle, I guess the job (eventually...) needs some sort of 'discard' edges in the state machine, so that the dispatcher can cleanly abandon jobs. https://codereview.chromium.org/2193813002/diff/1/test/unittests/compiler-dis... File test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc (right): https://codereview.chromium.org/2193813002/diff/1/test/unittests/compiler-dis... test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:76: ASSERT_TRUE(job->can_parse_on_background_thread()); How can this still work? I don't think this var is being set?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9960) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher-job.cc (right): https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher-job.cc:144: character_stream_.reset(); On 2016/07/29 at 12:46:18, vogelheim wrote: > [general comment:] > > Since this class handles more & more resources, IMHO you should in the destructor either: > - assert a proper terminal state (kFailed or kCompiled) > - or enforce cleanup in the destructor. > > I prefer the former, since I guess the intention is that the dispatcher should run the state machine to its proper end (& hence the job-class can assume it will go through the proper life cycle). > > Loosely related: If the intention is that the dispatcher controls the lifecycle, I guess the job (eventually...) needs some sort of 'discard' edges in the state machine, so that the dispatcher can cleanly abandon jobs. yeah, that makes sense https://codereview.chromium.org/2193813002/diff/1/test/unittests/compiler-dis... File test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc (right): https://codereview.chromium.org/2193813002/diff/1/test/unittests/compiler-dis... test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:76: ASSERT_TRUE(job->can_parse_on_background_thread()); On 2016/07/29 at 12:46:18, vogelheim wrote: > How can this still work? I don't think this var is being set? it's set in the ctor of the job
The CQ bit was checked by jochen@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...
added ResetOnMainThread() and checks in dtor
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Otherwise lgtm https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... File src/compiler-dispatcher/compiler-dispatcher-job.cc (right): https://codereview.chromium.org/2193813002/diff/1/src/compiler-dispatcher/com... src/compiler-dispatcher/compiler-dispatcher-job.cc:58: source_ = Handle<String>::cast(isolate_->global_handles()->Create(*source)); On 2016/07/29 12:23:31, jochen wrote: > have to globalize the reference here, so it survives between function calls How about putting this comment into the source code :) https://codereview.chromium.org/2193813002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher-job.cc (right): https://codereview.chromium.org/2193813002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher-job.cc:116: InternalizeParsingResult(); Wouldn't it make sense to always call InternalizeParsingResult here, since otherwise you need to call it in ReportErrorsOnMainThread anyway?
https://codereview.chromium.org/2193813002/diff/20001/src/compiler-dispatcher... File src/compiler-dispatcher/compiler-dispatcher-job.cc (right): https://codereview.chromium.org/2193813002/diff/20001/src/compiler-dispatcher... src/compiler-dispatcher/compiler-dispatcher-job.cc:116: InternalizeParsingResult(); On 2016/08/01 at 07:50:24, marja wrote: > Wouldn't it make sense to always call InternalizeParsingResult here, since otherwise you need to call it in ReportErrorsOnMainThread anyway? InternalizeParsingResult will schedule an exception on the isolate in case of an error. I want to have full control over when the exception gets scheduled
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2193813002/#ps40001 (title: "updates")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add parser finalization step BUG=v8:5215 R=vogelheim@chromium.org,marja@chromium.org ========== to ========== Add parser finalization step BUG=v8:5215 R=vogelheim@chromium.org,marja@chromium.org Committed: https://crrev.com/f22ef1207d731f782dafb8872a1afe7401d323b5 Cr-Commit-Position: refs/heads/master@{#38203} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f22ef1207d731f782dafb8872a1afe7401d323b5 Cr-Commit-Position: refs/heads/master@{#38203} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
