Track loading happening in parallel with background compilation and abort compilation if necessary.

Track loading happening in parallel with background compilation and abort compilation if necessary.

BUG=
R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/f60e86fd83a1c97773509b5255f34096179d54db

[srdjan, 2016-03-24 22:48:31]

srdjan@google.com changed reviewers:
+ asiva@google.com

[srdjan, 2016-03-25 21:57:19]

PTAL

[siva, 2016-03-28 17:20:14]

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1158: if (isolate->top_level_parsing() ||
top level parsing is set in the mutator thread and accessed here without any
locks, seems racy.

Is this check needed, won't top level parsing result in the invalidation_gen
value being incremented?

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_inliner.cc:710: if (isolate->top_level_parsing() ||
This value is set in the mutator thread and accessed here in the background
compiler thread, seems racy.

Do you need this value, isn't tracking loading_invalidation_gen_ sufficient as
you seem to increment that too when top level parsing is done.

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:506: if (loading_invalidation_gen_ == kInvalidGen)
loading_invalidation_gen_++;
Why are you first incrementing and then checking, won't the check always fail?

Since these values are accessed from another thread, I am wondering if we should
be doing atomic increment operations here.

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:505: }
Both constructor and destructor are incrementing the loading_invalidation_gen_
value, is that needed? Do you want to differentiate between start of top level
parsing and end?

[srdjan, 2016-03-28 19:54:54]

PTAL

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1158: if (isolate->top_level_parsing() ||
On 2016/03/28 17:20:14, siva wrote:
> top level parsing is set in the mutator thread and accessed here without any
> locks, seems racy.
> 
> Is this check needed, won't top level parsing result in the invalidation_gen
> value being incremented?

As discussed, we need to make top_level_parsing flag and increment counters
atomically updated.
We need this check in case loading takes longer than compilation.

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_inliner.cc:710: if (isolate->top_level_parsing() ||
On 2016/03/28 17:20:14, siva wrote:
> This value is set in the mutator thread and accessed here in the background
> compiler thread, seems racy.
> 
> Do you need this value, isn't tracking loading_invalidation_gen_ sufficient as
> you seem to increment that too when top level parsing is done.

dito

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:506: if (loading_invalidation_gen_ == kInvalidGen)
loading_invalidation_gen_++;
On 2016/03/28 17:20:14, siva wrote:
> Why are you first incrementing and then checking, won't the check always fail?
> 
> Since these values are accessed from another thread, I am wondering if we
should
> be doing atomic increment operations here.

Since we never reset the count, we may overflow and encounter kInvalidGen again.
Removed unused ResetXXGen.

Yes for atomic operations.

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1834763004/diff/100001/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:505: }
On 2016/03/28 17:20:14, siva wrote:
> Both constructor and destructor are incrementing the loading_invalidation_gen_
> value, is that needed? Do you want to differentiate between start of top level
> parsing and end?

I do not need to differentiate. The only information I need is that the a
loading occurred. In that context, I only need to increment the loading counter
at the exit, since the start and loading is covered by the IsTopLevelParsing
state/check.

[siva, 2016-03-28 22:00:09]

LGTM with one comment about using LoadRelaxed the way we do in other places.

[srdjan, 2016-03-28 23:02:44]

Description was changed from

==========
Track loading happening in parallel with background compilation and abort
compilation if necessary.

BUG=
==========

to

==========
Track loading happening in parallel with background compilation and abort
compilation if necessary.

BUG=
R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/f60e86fd83a1c97773509b5255f34096179d54db
==========

[srdjan, 2016-03-28 23:02:45]

Committed patchset #9 (id:160001) manually as
f60e86fd83a1c97773509b5255f34096179d54db (presubmit successful).