Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Shut down and restart the Dart VM as needed. #6387

Closed
wants to merge 7 commits into from
Closed

Shut down and restart the Dart VM as needed. #6387

wants to merge 7 commits into from

Conversation

chinmaygarde
Copy link
Member

The shell was already designed to cleanly shut down the VM but it couldn't
earlier as Dart_Initialize could never be called again after a Dart_Cleanup. This
meant that shutting down an engine instance could not shut down the VM to save
memory because newly created engines in the process after that point couldn't
restart the VM. There can only be one VM running in a process at a time.

This limitation was lifted in https://dart-review.googlesource.com/c/sdk/+/76561.

@chinmaygarde
Copy link
Member Author

chinmaygarde commented Sep 28, 2018 •
edited
Loading

Cannot land this ATM as https://dart-review.googlesource.com/c/sdk/+/76561 has not been rolled in. The DartVM tests will crash with this patch applied but the dart-roll not completed.

@chinmaygarde chinmaygarde mentioned this pull request Sep 28, 2018
Closed
cbracken
cbracken reviewed Sep 28, 2018
View reviewed changes
runtime/dart_vm.cc Outdated
return gVM;

// TODO(chinmaygarde): Make this an assertion instead so that callers are
// notified that the new settings might not hold becuase an exiting VM was
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: because

cbracken
cbracken reviewed Sep 28, 2018
View reviewed changes
runtime/dart_vm.cc Outdated
return gVM;

// TODO(chinmaygarde): Make this an assertion instead so that callers are
// notified that the new settings might not hold because an exiting VM was
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: existing

cbracken
cbracken approved these changes Sep 28, 2018
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@chinmaygarde
Copy link
Member Author

The patch in dart-lang/sdk seems to have been reverted. Stashing this for now till the issues are resolved.

@chinmaygarde chinmaygarde force-pushed the shutdown_vm branch 2 times, most recently from 4c43db7 to 6d4b65b Compare October 4, 2018 00:29
@chinmaygarde
Copy link
Member Author

More issues have been found in the VM while attempting a restart (described here).

@chinmaygarde chinmaygarde force-pushed the shutdown_vm branch 2 times, most recently from 2add29d to 3f9387a Compare October 10, 2018 00:41
@chinmaygarde
Copy link
Member Author

I am still unable to set flags at the ToT Dart VM. Dart_SetVMFlags says the "Flags already set".

@Hixie Hixie mentioned this pull request Oct 31, 2018
Closed
@chinmaygarde chinmaygarde force-pushed the shutdown_vm branch 2 times, most recently from 541364d to 4f185c5 Compare November 15, 2018 01:00
@Hixie
Copy link
Contributor

Hixie commented Dec 19, 2018

@a-siva Any update on the Dart team trying to land this?

@a-siva
Copy link
Contributor

a-siva commented Dec 19, 2018

This is fixed in the bleeding edge Dart sources and if we can get a roll of Dart into flutter we should be able to verify the fixes.

@chinmaygarde
Copy link
Member Author

A couple of rolls have landed. Retrying.

@chinmaygarde
Copy link
Member Author

@a-siva There still seems to be a race when the VM is shutdown immediately after launch. Running the runtime_unittests target crashes with the following error and backtrace in about 1 in 3 runs. When successful, all 2000 odd relaunches of the VM are fine. I am on OSX Mojave BTW.

Error:

../../third_party/dart/runtime/vm/os_thread_macos.cc: 356: error: pthread error: 22 (Invalid argument)

Backtrace:

* thread #2, stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x00000001018ee3e1 runtime_unittests`dart::Profiler::DumpStackTrace(unsigned long, unsigned long, unsigned long, bool) [inlined] dart::OSThread::trace_id(this=<unavailable>) const at os_thread.h:74
    frame #1: 0x00000001018ee3e1 runtime_unittests`dart::Profiler::DumpStackTrace(sp=123145441745184, fp=123145441745264, pc=4321108613, for_crash=<unavailable>) at profiler.cc:1117
    frame #2: 0x0000000101d56bb2 runtime_unittests`dart::Assert::Fail(this=<unavailable>, format=<unavailable>) at assert.cc:42
    frame #3: 0x00000001018ea470 runtime_unittests`dart::Monitor::Enter(this=<unavailable>) at os_thread_macos.cc:356
    frame #4: 0x00000001019a2350 runtime_unittests`dart::ThreadPool::Worker::Main(unsigned long) [inlined] dart::MonitorLocker::MonitorLocker(this=<unavailable>, monitor=0x0000000109b01270, no_safepoint_scope=true) at lockers.h:139
    frame #5: 0x00000001019a2348 runtime_unittests`dart::ThreadPool::Worker::Main(unsigned long) [inlined] dart::MonitorLocker::MonitorLocker(this=<unavailable>, monitor=0x0000000109b01270, no_safepoint_scope=true) at lockers.h:127
    frame #6: 0x00000001019a2348 runtime_unittests`dart::ThreadPool::Worker::Main(args=4457506128) at thread_pool.cc:464
    frame #7: 0x00000001018e98f9 runtime_unittests`dart::ThreadStart(data_ptr=<unavailable>) at os_thread_macos.cc:107
    frame #8: 0x000000010541de35 libsystem_pthread.dylib`_pthread_body + 126
    frame #9: 0x0000000105420ec7 libsystem_pthread.dylib`_pthread_start + 70
    frame #10: 0x000000010541ce51 libsystem_pthread.dylib`thread_start + 13

@a-siva
Copy link
Contributor

a-siva commented Jan 10, 2019

Will take a look.

@chinmaygarde
Copy link
Member Author

@a-siva Thanks! I have rebased the branch to ToT and pushed the update. You should just be able to pull and run the unit-test target.

@a-siva
Copy link
Contributor

a-siva commented Feb 9, 2019

With this PR it appears that Dart_Cleanup gets called from within the isolate create callback function when the service isolate is being created. We do not expect Dart_Cleanup to be called from within the create callback (there should have been a check and error message for this).
Calling Dart_Cleanup from within that callback leads to the thread pool being shutdown and destroyed while a thread pool thread was still bringing up the service isolate.

@chinmaygarde
Copy link
Member Author

Thanks! I did not notice this was in the create callback since that was on another thread. This means there is a legitimate race condition. I'll work on fixing isolate VM lifecycle to avoid this issue.

@chinmaygarde
Shut down and restart the Dart VM as needed.
5d38170 
The shell was already designed to cleanly shut down the VM but it couldnt
earlier as Dart_Initialize could never be called after a Dart_Cleanup. This
meant that shutting down an engine instance could not shut down the VM to save
memory because newly created engines in the process after that point couldn't
restart the VM. There can only be one VM running in a process at a time.
@chinmaygarde
Copy link
Member Author

I am reworking DartVM lifecycle in a major way after talking to @a-siva. Will open another PR when done.

@chinmaygarde
Copy link
Member Author

The new version of this PR is #7832. Disregarding the LGTM here since that version is substantially different from this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbracken cbracken cbracken approved these changes

@rmacnak-google rmacnak-google Awaiting requested review from rmacnak-google

@jason-simmons jason-simmons Awaiting requested review from jason-simmons

@bkonyi bkonyi Awaiting requested review from bkonyi

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chinmaygarde @Hixie @a-siva @cbracken @googlebot