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.

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

Switch from remove callbacks to passing an AbortSignal aborted after removal + accepting a removal promise. #2756

Closed
dead-claudia opened this issue Feb 26, 2022 · 1 comment
Assignees
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Feb 26, 2022

Description

The idea is this: replace onremove and onbeforeremove with:

  • An AbortSignal parameter to component creation and views
  • A removeReady lifecycle "hook" that's a simple static promise (checking this is as simple as != null and we wouldn't need to call anything, simplifying our side)

Old:

var FancyComponent = {
	onbeforeremove(vnode) {
		vnode.dom.classList.add("exit")
		return new Promise(function(resolve) {
			vnode.dom.addEventListener("animationend", resolve)
		})
	},
	view: () => m(".fancy", "Hello world"),
}

function Async(v) {
	let method = "pending"
	const ctrl = new AbortController()
	let data
	new Promise(resolve => resolve(v.attrs.init(ctrl.signal))).then(
		d => { method = "ready"; data = d; m.redraw() },
		e => { method = "error"; data = e; m.redraw() }
	)
	return {
		view(v) {
			let view = v.attrs[method](
				method === "pending" ? undefined : data
			)
			if (!Array.isArray(view)) view = [view]
			return m.fragment({key: method}, view)
		},
		onremove() {
			if (method === "pending") ctrl.abort()
		},
	}
}

New:

This also includes a simple m.aborted(signal) helper

function FancyComponent(_, signal) {
	let onanimationend, dom
   	return {
		removeReady: new Promise(resolve => onanimationend = resolve),
		view: () => m(".fancy", {
			oncreate(v) { signal.onabort = () => v.dom.classList.add("exit") },
			onanimationend,
		}, "Hello world"),
	}
}

function Async(v, signal) {
	let method = "pending"
	const ctrl = new AbortController()
	let data
	new Promise(resolve => resolve(v.attrs.init(signal))).then(
		d => { method = "ready"; data = d; m.redraw() },
		e => { method = "error"; data = e; m.redraw() }
	)
	return {
		view(v) {
			let view = v.attrs[method](
				method === "pending" ? undefined : data
			)
			if (!Array.isArray(view)) view = [view]
			return m.fragment({key: method}, view)
		},
	}
}

Why

Possible Implementation

Our remove logic would, instead of invoking remove callbacks, do the following:

  • If the vnode has a removeReady promise and this isn't being run from within a .then handler, skip the vnode.
  • Schedule a ctrl.abort.bind(ctrl) to run alongside lifecycle hooks
  • Visit all the vnode's children using this same recursive algorithm

Creation logic would create the above abort controller, and this would be stored with the vnode.

Open Questions

Do we really want to add a whole vnode field just to support this? Our vnodes are already rather bloated, and the vast majority of our memory usage and GC pressure comes from our vnodes.

I found in Chrome that a single new AbortController() call allocates 188 bytes each (60 for the controller, 128 for the signal). This combined with the slot (4 bytes) comes out to 192 bytes of overhead just for the abort controller alone. That doesn't sit well in light of #2755.

@dead-claudia dead-claudia added Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Feb 26, 2022
@dead-claudia dead-claudia self-assigned this Feb 26, 2022
@dead-claudia
Copy link
Member Author

This conflicts with #2690 and as #2282 provides an abort signal anyways, the benefit is too small to outweigh the potential GC performance risk and implementation complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
None yet
Development

No branches or pull requests

1 participant