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

simplify safe_not_equal and not_equal? #2886

Open
Rich-Harris opened this issue May 27, 2019 · 10 comments
Open

simplify safe_not_equal and not_equal? #2886

Rich-Harris opened this issue May 27, 2019 · 10 comments
Labels
compiler Changes relating to the compiler
Milestone

Comments

@Rich-Harris
Copy link
Member

I was doing some profiling the other day and was mildly surprised to find that a lot of the time spent in Svelte — possibly even most? — was in safe_not_equal. It occurs to me that we might see some performance gains by changing the implementations thusly:

+function is_mutable(type) {
+	return type === 'object' || type === 'function';
+}
+
export function safe_not_equal(a, b) {
-	return a != a ? b == b : a !== b || ((a && typeof a === 'object') || typeof a === 'function');
+	return a !== b || a && is_mutable(typeof a);
}

export function not_equal(a, b) {
-	return a != a ? b == b : a !== b;
+	return a !== b;
}

The downside is that setting x to NaN would trigger an update if x was already NaN. But I'm pretty sure that's a small enough edge case that we needn't worry about it

@Conduitry
Copy link
Member

This seems reasonable.

Is there much of a performance gain in putting is_mutable into its own function? Does losing one typeof and gaining a function call help?

@karuppasamy
Copy link

I am not arguing. But, I would suggest keeping the method not_equal as it is to handle NaN as this is a special case may raise a performance issue if missed.

I have changed fnis_mutable little bit to accept an object instead of the typeof of the same as this makes sense of this case.

export function is_mutable(obj) {
  return typeof obj === 'object' || typeof obj === 'function'
}

export function safe_not_equal(a, b) {
  return not_equal(a, b) || is_mutable(a)
}

export function not_equal(a, b) {
  return a != a ? b == b : a !== b;
}

@Rich-Harris
Copy link
Member Author

Using this snippet...

const obj = {};
const foo = function () { }

function safe_not_equal(a, b) {
	return a != a ? b == b : a !== b || ((a && typeof a === 'object') || typeof a === 'function');
}

function safe_not_equal_simplified(a, b) {
	return a !== b || (a && (typeof a === 'object' || typeof a === 'function'));
}

function is_mutable(type) {
	return type === 'object' || type === 'function';
}

function safe_not_equal_simplified_indirect(a, b) {
	return a !== b || (a && is_mutable(typeof a));
}

// --------------------

const iterations = 1e7;

function test(fn, a, b) {
	console.time(fn.name);
	let i = iterations;
	while (i--) {
		const object_differs = fn(a, b);
	}
	console.timeEnd(fn.name);
}

...then running things like test(safe_not_equal, obj, obj), I get a clear answer — safe_not_equal_simplified is the quickest. It seems that the function call overhead is more significant than I imagined at this level of microoptimisation — the last version (safe_not_equal_simplified_indirect) is a little bit slower in Chrome, and an extraordinary amount slower in Firefox.

@karuppasamy The whole point of this issue is to increase performance! As discussed, NaN is a rare edge case. Your implementation makes everything slower — it keeps the NaN check but adds a function call, keeps the is_mutable function call and keeps the double typeof check.

@trbrc
Copy link
Contributor

trbrc commented May 28, 2019

Just a question. The most important thing to optimize is when they are equal, correct? Because if they’re not, that leads to an update that probably makes any microoptimization dwarf in comparison? I might be missing something, but if that’s true, there’s no reason to do a NaN check when a === b anyway, and little harm in doing it when a !== b?

@Rich-Harris
Copy link
Member Author

@trbrc to be clear, is this what you're proposing?

function safe_not_equal(a, b) {
	return a === b
		? (a ? (typeof a === 'object' || typeof a === 'function') : false)
		: a === a && b === b;
}

@trbrc
Copy link
Contributor

trbrc commented May 29, 2019

@Rich-Harris Yes, something very much like it. I have no idea where it sits with real world performance, but it looks to me like it should keep the original semantics but with less work to return false, and very similar work to return true. Do you agree?

@mrkishi
Copy link
Member

mrkishi commented May 29, 2019

@trbrc's proposal is solid, imo. You just got a little bug with NaNs, but once corrected it is the overall winner on my machine.

I also tried it without the polymorphic test(fn) since Svelte always references the same function throughout, and was surprised by how much close they all are in FF on Windows for me.

Little live test case if anyone also wants to check it out:

@stale
Copy link

stale bot commented Dec 27, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Malix-Labs
Copy link

Malix-Labs commented Jan 2, 2024

The downside is that setting x to NaN would trigger an update if x was already NaN.

That is the current behavior in v5 Runes btw.

Update

I discovered there is an inconsistency between v5 Classic and v5 Runes about updates being triggered by re-assigning NaN to an already NaN variable.
As it is thus probably not intended, I opened #10061 about it.

@Malix-Labs
Copy link

Malix-Labs commented Jan 2, 2024

@Rich-Harris Rich-Harris added this to the 5.x milestone Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Changes relating to the compiler
Projects
None yet
Development

No branches or pull requests

8 participants