This repository has been archived by the owner. It is now read-only.

vm: revert contextify/vm2 patches #6228

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@bnoordhuis
Member

bnoordhuis commented Sep 13, 2013

The new contextify-based vm2 implementation has some teething issues,
notably that it leaks persistent handles and is prone to segmentation
faults and/or debug build V8 assertions because it doesn't manage the
lifecycle of persistent handles correctly.

After some deliberation, I've come to the conclusion that the issues
are pretty fundamental to how the vm2 module is structured and that
fixing them properly will require a fair amount of rework.

With v0.12 around the corner, there is probably not enough time to get
things right. That's why this commit reverts or partially reverts the
following commits, in reverse chronological order:

3d4c663 contextify: dealloc only after global and sandbox
b89b97d src: fix multi-base class ObjectWrap::Unwrap()
756b622 src: add multi-context support
fd36576 vm: update API to use options argument
9c110d8 vm: add isContext; prevent double-contextifying
a3bf3d1 vm: use MakeWeak to fix leaking contexts
a54f65c vm: rip out ObjectWrap from ContextifyContext
9fc0066 src: fix up unused/unordered imports
48976d2 vm: fix Persistent leak
2891790 vm: remove unnecessary Persistent
eef5527 vm: Put back display_errors flag
7afdba6 vm, core, module: re-do vm to fix known issues

Some fix-ups were applied to the restored src/node_script.cc to make it
work with changed internal APIs. It still needs to be made fully
compatible with the multi-context work from commit 756b622.

Conflicts:
src/node_contextify.cc
lib/repl.js

/cc @domenic @isaacs @indutny

vm: revert contextify/vm2 patches
The new contextify-based vm2 implementation has some teething issues,
notably that it leaks persistent handles and is prone to segmentation
faults and/or debug build V8 assertions because it doesn't manage the
lifecycle of persistent handles correctly.

After some deliberation, I've come to the conclusion that the issues
are pretty fundamental to how the vm2 module is structured and that
fixing them properly will require a fair amount of rework.

With v0.12 around the corner, there is probably not enough time to get
things right.  That's why this commit reverts or partially reverts the
following commits, in reverse chronological order:

  3d4c663 contextify: dealloc only after global and sandbox
  b89b97d src: fix multi-base class ObjectWrap::Unwrap<T>()
  756b622 src: add multi-context support
  fd36576 vm: update API to use options argument
  9c110d8 vm: add isContext; prevent double-contextifying
  a3bf3d1 vm: use MakeWeak to fix leaking contexts
  a54f65c vm: rip out ObjectWrap from ContextifyContext
  9fc0066 src: fix up unused/unordered imports
  48976d2 vm: fix Persistent<Context> leak
  2891790 vm: remove unnecessary Persistent<FunctionTemplate>
  eef5527 vm: Put back display_errors flag
  7afdba6 vm, core, module: re-do vm to fix known issues

Some fix-ups were applied to the restored src/node_script.cc to make it
work with changed internal APIs.  It still needs to be made fully
compatible with the multi-context work from commit 756b622.

Conflicts:
	src/node_contextify.cc
	lib/repl.js
@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Sep 13, 2013

Member

I would love to be able to prove you wrong, pull an all-nighter and fix all the bugs without much effort, and then say we should keep it. I think it should in theory be fixable, since contextify itself doesn't have these problems (I'm pretty sure), so they were introduced in the porting. At the very least I'd be curious if reverting the multi-context stuff fixed any of the crashes.

But unfortunately I'm at JSConf EU this weekend, and headed to a TC39 meeting after I get back, i.e. I'll be unable to devote concerted time to this until roughly the 21st. If 0.12 is indeed as imminent as you say, and nobody else has the bandwidth to try to fix this before then, I can't really see what else to do.

That said, it would be a real shame to lose vm2 from 0.12, as people keep approaching me in person thanking me for it -_-. I think to some extent it's a headlining feature, at least for those people. Pushing it to 1.0 would be a small tragedy.

Member

domenic commented Sep 13, 2013

I would love to be able to prove you wrong, pull an all-nighter and fix all the bugs without much effort, and then say we should keep it. I think it should in theory be fixable, since contextify itself doesn't have these problems (I'm pretty sure), so they were introduced in the porting. At the very least I'd be curious if reverting the multi-context stuff fixed any of the crashes.

But unfortunately I'm at JSConf EU this weekend, and headed to a TC39 meeting after I get back, i.e. I'll be unable to devote concerted time to this until roughly the 21st. If 0.12 is indeed as imminent as you say, and nobody else has the bandwidth to try to fix this before then, I can't really see what else to do.

That said, it would be a real shame to lose vm2 from 0.12, as people keep approaching me in person thanking me for it -_-. I think to some extent it's a headlining feature, at least for those people. Pushing it to 1.0 would be a small tragedy.

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Sep 13, 2013

Member

The multi-context work is unrelated. The issues are caused by the fact that there are several weak persistent handles that reference each other and their lifecycles are not managed correctly.

Segmentation faults are the most obvious way that expresses itself but it also leads to memory leaks and those are a lot harder to confirm and track down.

The proper solution is probably to turn those persistent handles into object groups but that takes a fair amount of re-architecting core infrastructure. There's probably also the performance impact to consider because you have to re-register the object group before every major GC cycle.

That said, if someone steps up and fixes the issues, good. But I'll probably be offline for a week or more after this weekend, I won't be around to do it or review patches. If we want to release v0.12 this month, then reverting vm2 seems prudent. The people that were looking forward to it will get over it.

Member

bnoordhuis commented Sep 13, 2013

The multi-context work is unrelated. The issues are caused by the fact that there are several weak persistent handles that reference each other and their lifecycles are not managed correctly.

Segmentation faults are the most obvious way that expresses itself but it also leads to memory leaks and those are a lot harder to confirm and track down.

The proper solution is probably to turn those persistent handles into object groups but that takes a fair amount of re-architecting core infrastructure. There's probably also the performance impact to consider because you have to re-register the object group before every major GC cycle.

That said, if someone steps up and fixes the issues, good. But I'll probably be offline for a week or more after this weekend, I won't be around to do it or review patches. If we want to release v0.12 this month, then reverting vm2 seems prudent. The people that were looking forward to it will get over it.

@indutny

This comment has been minimized.

Show comment Hide comment
@indutny

indutny Sep 13, 2013

Member

@bnoordhuis if you'll describe all current issues properly, I may look into fixing them during next week. If I'll fail in this, we can revert it once you'll get back from your stuff.

Member

indutny commented Sep 13, 2013

@bnoordhuis if you'll describe all current issues properly, I may look into fixing them during next week. If I'll fail in this, we can revert it once you'll get back from your stuff.

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Sep 14, 2013

Member

@indutny Okay, go for it.

Member

bnoordhuis commented Sep 14, 2013

@indutny Okay, go for it.

@indutny

This comment has been minimized.

Show comment Hide comment
@indutny

indutny Sep 14, 2013

Member

@bnoordhuis looks like joyent#6232 is fixing both crash and memory leak.

Member

indutny commented Sep 14, 2013

@bnoordhuis looks like joyent#6232 is fixing both crash and memory leak.

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