Teach compiler jobs how to actually parse

Teach compiler jobs how to actually parse

BUG=v8:5215
R=marja@chromium.org,vogelheim@chromium.org

Committed: https://crrev.com/edd65cc4286fab89d71f80bfda6e6c35be5c4543
Cr-Commit-Position: refs/heads/master@{#38170}

[jochen (gone - plz use gerrit), 2016-07-29 09:41:52]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 09:41:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2016-07-29 09:55:58]

lgtm

[jochen (gone - plz use gerrit), 2016-07-29 09:58:26]

The CQ bit was unchecked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-07-29 09:58:28]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2016-07-29 09:58:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 10:06:01]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-29 10:09:32]

Description was changed from

==========
Teach compiler jobs how to actually parse

BUG=v8:5215
R=marja@chromium.org,vogelheim@chromium.org
==========

to

==========
Teach compiler jobs how to actually parse

BUG=v8:5215
R=marja@chromium.org,vogelheim@chromium.org

Committed: https://crrev.com/edd65cc4286fab89d71f80bfda6e6c35be5c4543
Cr-Commit-Position: refs/heads/master@{#38170}
==========

[commit-bot: I haz the power, 2016-07-29 10:09:33]

Patchset 1 (id:??) landed as
https://crrev.com/edd65cc4286fab89d71f80bfda6e6c35be5c4543
Cr-Commit-Position: refs/heads/master@{#38170}

[vogelheim, 2016-07-29 10:19:56]

lgtm

I don't get the concurrency stuff, though.

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.cc (right):

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:30: if
(source->IsExternalTwoByteString()) {
nitpick:

can_parse_on_background_thread_ = source->IsExternalOneByteString() ||
source->isExternalTwoByteString();

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:80: DCHECK(status() ==
CompileJobStatus::kReadyToParse);
I don't understand the concurrency model.

- You've made status_ an atomic variable, so you expect it to be called without
any external synchronization.
- Nothing else here is lock-guarded or atomic, so clearly non of the rest
expects to be called concurrently, but assumes that the caller has made sure
this object is 'theirs'.
- Presumably, the caller would make some sort of decisions based on status().
-  But.. you only update it at the end, which looks like there might be a race
condition. That is, something has called Parse(), and another is calling
status(), presumably to determine whether it may call Parse() also.

This doesn't seem to fit together... care to explain?

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.h (right):

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.h:51:
base::AtomicValue<CompileJobStatus>(CompileJobStatus::kInitial);
Why initialize here, and not in constructor?

[jochen (gone - plz use gerrit), 2016-07-29 10:27:45]

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.cc (right):

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:30: if
(source->IsExternalTwoByteString()) {
On 2016/07/29 at 10:19:56, vogelheim wrote:
> nitpick:
> 
> can_parse_on_background_thread_ = source->IsExternalOneByteString() ||
source->isExternalTwoByteString();

will update in my next cl

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:80: DCHECK(status() ==
CompileJobStatus::kReadyToParse);
On 2016/07/29 at 10:19:56, vogelheim wrote:
> I don't understand the concurrency model.
> 
> - You've made status_ an atomic variable, so you expect it to be called
without any external synchronization.
> - Nothing else here is lock-guarded or atomic, so clearly non of the rest
expects to be called concurrently, but assumes that the caller has made sure
this object is 'theirs'.
> - Presumably, the caller would make some sort of decisions based on status().
> -  But.. you only update it at the end, which looks like there might be a race
condition. That is, something has called Parse(), and another is calling
status(), presumably to determine whether it may call Parse() also.
> 
> This doesn't seem to fit together... care to explain?

Yeah, I guess I can make it non-atomic and just introduce a lock to be held once
the dispatcher exists and can invoke stuff on several threads

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.h (right):

https://codereview.chromium.org/2190333002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.h:51:
base::AtomicValue<CompileJobStatus>(CompileJobStatus::kInitial);
On 2016/07/29 at 10:19:56, vogelheim wrote:
> Why initialize here, and not in constructor?

so you see in the header that this is independend of the ctor parameters