- Assert that we do not hold the timeline lock while entering

- Assert that we do not hold the timeline lock while entering
  potential safepoints.
- Enable timeline for existing test as a regression test.
- Fix allocation with timeline lock held during deoptimization to
  prevent deadlock.

BUG=
R=rmacnak@google.com

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

[Ivan Posva, 2015-12-09 20:08:02]

iposva@google.com changed reviewers:
+ fschneider@google.com, rmacnak@google.com

[rmacnak, 2015-12-09 21:40:05]

On 2015/12/09 20:08:02, Ivan Posva wrote:

LGTM

(+ John's LGTM on email but not on Reitveld)

[Ivan Posva, 2015-12-09 21:41:46]

Description was changed from

==========
- Assert that we do not hold the timeline lock while entering
  potential safepoints.
- Enable timeline for existing test as a regression test.
- Fix allocation with timeline lock held during deoptimization to
  prevent deadlock.

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

to

==========
- Assert that we do not hold the timeline lock while entering
  potential safepoints.
- Enable timeline for existing test as a regression test.
- Fix allocation with timeline lock held during deoptimization to
  prevent deadlock.

BUG=
R=rmacnak@google.com

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

[Ivan Posva, 2015-12-09 21:41:47]

Committed patchset #1 (id:1) manually as
f887b03b5b21735a292ef599d132568b661ce29b (presubmit successful).

[Florian Schneider, 2015-12-10 17:01:46]

This seems still fragile to me: Couldn't the same issue arise for any other two
TimelineDurationScopes (e.g. not involving GC)?

https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
File tests/standalone/array_bounds_check_generalization_test.dart (right):

https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
tests/standalone/array_bounds_check_generalization_test.dart:7: //
VMOptions=--optimization_counter_threshold=10 --no-use-osr --complete-timeline
Could you reproduce the deadlock with this test? I could not.

[rmacnak, 2015-12-10 17:18:21]

On 2015/12/10 17:01:46, Florian Schneider wrote:
> This seems still fragile to me: Couldn't the same issue arise for any other
two
> TimelineDurationScopes (e.g. not involving GC)?

TimelineDurationScope only holds the lock in its destructor, not for its whole
lifetime.


>
https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
> File tests/standalone/array_bounds_check_generalization_test.dart (right):
> 
>
https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
> tests/standalone/array_bounds_check_generalization_test.dart:7: //
> VMOptions=--optimization_counter_threshold=10 --no-use-osr --complete-timeline
> Could you reproduce the deadlock with this test? I could not.

[Florian Schneider, 2015-12-10 17:52:49]

On 2015/12/10 17:18:21, rmacnak wrote:
> On 2015/12/10 17:01:46, Florian Schneider wrote:
> > This seems still fragile to me: Couldn't the same issue arise for any other
> two
> > TimelineDurationScopes (e.g. not involving GC)?
> 
> TimelineDurationScope only holds the lock in its destructor, not for its whole
> lifetime.
>

Oh, yes right. Wouldn't it then be nicer to use TimelineDurationScope in
DeoptContext as well?
 
> 
> >
>
https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
> > File tests/standalone/array_bounds_check_generalization_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
> > tests/standalone/array_bounds_check_generalization_test.dart:7: //
> > VMOptions=--optimization_counter_threshold=10 --no-use-osr
--complete-timeline
> > Could you reproduce the deadlock with this test? I could not.

[Ivan Posva, 2015-12-10 18:07:21]

On 2015/12/10 17:52:49, Florian Schneider wrote:
> On 2015/12/10 17:18:21, rmacnak wrote:
> > On 2015/12/10 17:01:46, Florian Schneider wrote:
> > > This seems still fragile to me: Couldn't the same issue arise for any
other
> > two
> > > TimelineDurationScopes (e.g. not involving GC)?
> > 
> > TimelineDurationScope only holds the lock in its destructor, not for its
whole
> > lifetime.
> >
> 
> Oh, yes right. Wouldn't it then be nicer to use TimelineDurationScope in
> DeoptContext as well?

Except the TimelineDurationScope is a StackObject and deoptimization takes
several phases, so the DeoptContext needs to manually emulate what the
TimelineDurationScope convenience class does for you in the generally applicable
case.

-Ivan

>  
> > 
> > >
> >
>
https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
> > > File tests/standalone/array_bounds_check_generalization_test.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1514653002/diff/1/tests/standalone/array_boun...
> > > tests/standalone/array_bounds_check_generalization_test.dart:7: //
> > > VMOptions=--optimization_counter_threshold=10 --no-use-osr
> --complete-timeline
> > > Could you reproduce the deadlock with this test? I could not.