Skip to content

perf: save some bytes with more concise comparisons #10124

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

Closed
wants to merge 4 commits into from
Closed

perf: save some bytes with more concise comparisons #10124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72b3264
perf: save some bytes with more concise comparisons
benmccann Jan 8, 2024
87ac2a8
i guess there's more client code besides just runtime.js
benmccann Jan 9, 2024
0ed05cf
a few more
benmccann Jan 9, 2024
0f4c7e8
a few more
benmccann Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 56 additions & 59 deletions packages/svelte/src/internal/client/each.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ function no_op() {}
* @returns {void}
*/
function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, reconcile_fn) {
const is_controlled = (flags & EACH_IS_CONTROLLED) !== 0;
const is_controlled = !!(flags & EACH_IS_CONTROLLED);
Copy link
Member

@Conduitry Conduitry Jan 9, 2024

Choose a reason for hiding this comment

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

If is_controlled is only ever used in contexts where its truthiness matters rather than it being true or false (I don't know whether this is the case), then we could remove the !! as well.

Copy link
Member

@Conduitry Conduitry Jan 9, 2024

Choose a reason for hiding this comment

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

Oh, I see now this was referenced in #10124 (comment) - Perhaps we don't want to make that change.

Copy link
Member Author

@benmccann benmccann Jan 9, 2024

Choose a reason for hiding this comment

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

usually where I left !! it was to appease typescript in a quick an minimal way

Copy link

@mquandalle mquandalle Jan 9, 2024

Choose a reason for hiding this comment

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

@benmccann Have you tested Boolean(flags & EACH_IS_CONTROLLED) both in the runtime micro benchmarks and the gziped size? In term of readability I find the intent much more explicit that the !!() conversion, but I don't know about performance.

Copy link
Contributor

@trueadm trueadm Jan 9, 2024

Choose a reason for hiding this comment

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

To be honest the biggest culprit for perf in my findings wasn't the boolean conversion on flags, but rather on null values. If you look at the bytecode generated by V8, it's almost twice as many operations needed when doing boolean conversion on these fields (interestingly). So I'd be more leaning towards having an is_null function there, which is readable and also very fast.

Copy link
Member Author

@benmccann benmccann Jan 9, 2024

Choose a reason for hiding this comment

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

Hmm. But the "hybrid" option from the benchmark is consistently the slowest or 2nd slowest one for me, so I'm not sure I buy that it's faster and it's certainly much harder to write

Copy link
Contributor

@trueadm trueadm Jan 9, 2024

Choose a reason for hiding this comment

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

It was the fastest for me consistently with the first one. I also find it fine to write – I don't find writing explicit code to be of any benefit, especially if the type system isn't there to help you.

const block = create_each_block(flags, anchor_node);

/** @type {null | import('./types.js').Render} */
Expand All @@ -73,15 +73,13 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
transitions.add(transition);
transition.f(() => {
transitions.delete(transition);
if (transitions.size === 0) {
if (fallback.e !== null) {
if (fallback.d !== null) {
remove(fallback.d);
fallback.d = null;
}
destroy_signal(fallback.e);
fallback.e = null;
if (!transitions.size && fallback.e) {
if (fallback.d) {
remove(fallback.d);
fallback.d = null;
}
destroy_signal(fallback.e);
fallback.e = null;
}
});
};
Expand All @@ -98,12 +96,12 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
const effect = render_effect(
() => {
const dom = block.d;
if (dom !== null) {
if (dom) {
remove(dom);
block.d = null;
}
let anchor = block.a;
const is_controlled = (block.f & EACH_IS_CONTROLLED) !== 0;
const is_controlled = !!(block.f & EACH_IS_CONTROLLED);
if (is_controlled) {
anchor = empty();
block.a.appendChild(anchor);
Expand All @@ -122,7 +120,7 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
/** @param {import('./types.js').EachBlock} block */
const clear_each = (block) => {
const flags = block.f;
const is_controlled = (flags & EACH_IS_CONTROLLED) !== 0;
const is_controlled = !!(flags & EACH_IS_CONTROLLED);
const anchor_node = block.a;
reconcile_fn(array, block, anchor_node, is_controlled, render_fn, flags, true, keys);
};
Expand All @@ -136,24 +134,24 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
: maybe_array == null
? []
: Array.from(maybe_array);
if (key_fn !== null) {
if (key_fn) {
keys = array.map(key_fn);
} else if ((flags & EACH_KEYED) === 0) {
} else if (!(flags & EACH_KEYED)) {
array.map(no_op);
}
const length = array.length;
if (fallback_fn !== null) {
if (length === 0) {
if (block.v.length !== 0 || render === null) {
if (fallback_fn) {
if (!length) {
if (block.v.length || !render) {
clear_each(block);
create_fallback_effect();
return;
}
} else if (block.v.length === 0 && current_fallback !== null) {
} else if (!block.v.length && current_fallback) {
const fallback = current_fallback;
const transitions = fallback.s;
if (transitions.size === 0) {
if (fallback.d !== null) {
if (!transitions.size) {
if (fallback.d) {
remove(fallback.d);
fallback.d = null;
}
Expand All @@ -162,7 +160,7 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
}
}
}
if (render !== null) {
if (render) {
execute_effect(render);
}
},
Expand All @@ -175,15 +173,15 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
push_destroy_fn(each, () => {
const flags = block.f;
const anchor_node = block.a;
const is_controlled = (flags & EACH_IS_CONTROLLED) !== 0;
const is_controlled = !!(flags & EACH_IS_CONTROLLED);
let fallback = current_fallback;
while (fallback !== null) {
while (fallback) {
const dom = fallback.d;
if (dom !== null) {
if (dom) {
remove(dom);
}
const effect = fallback.e;
if (effect !== null) {
if (effect) {
destroy_signal(effect);
}
fallback = fallback.p;
Expand Down Expand Up @@ -263,14 +261,14 @@ function reconcile_indexed_array(
var b_blocks;
var block;

if (active_transitions.length !== 0) {
if (active_transitions.length) {
destroy_active_transition_blocks(active_transitions);
}

if (b === 0) {
if (!b) {
b_blocks = [];
// Remove old blocks
if (is_controlled && a !== 0) {
if (is_controlled && a) {
clear_text_content(dom);
}
while (index < length) {
Expand All @@ -280,7 +278,7 @@ function reconcile_indexed_array(
} else {
var item;
b_blocks = Array(b);
if (current_hydration_fragment !== null) {
if (current_hydration_fragment) {
/** @type {Node} */
var hydrating_node = current_hydration_fragment[0];
for (; index < length; index++) {
Expand Down Expand Up @@ -350,7 +348,7 @@ function reconcile_tracked_array(
keys
) {
var a_blocks = each_block.v;
const is_computed_key = keys !== null;
const is_computed_key = !!keys;
var active_transitions = each_block.s;

/** @type {number | void} */
Expand All @@ -363,17 +361,17 @@ function reconcile_tracked_array(
var b_blocks;
var block;

if (active_transitions.length !== 0) {
if (active_transitions.length) {
destroy_active_transition_blocks(active_transitions);
}

if (b === 0) {
if (!b) {
b_blocks = [];
// Remove old blocks
if (is_controlled && a !== 0) {
if (is_controlled && a) {
clear_text_content(dom);
}
while (a > 0) {
while (a) {
block = a_blocks[--a];
destroy_each_item_block(block, active_transitions, apply_transitions, is_controlled);
}
Expand All @@ -384,12 +382,12 @@ function reconcile_tracked_array(
var item;
var idx;
b_blocks = Array(b);
if (current_hydration_fragment !== null) {
if (current_hydration_fragment) {
var fragment;

/** @type {Node} */
var hydrating_node = current_hydration_fragment[0];
while (b > 0) {
while (b) {
// Hydrate block
idx = b_end - --b;
item = array[idx];
Expand All @@ -406,9 +404,9 @@ function reconcile_tracked_array(
block = each_item_block(item, key, idx, render_fn, flags);
b_blocks[idx] = block;
}
} else if (a === 0) {
} else if (!a) {
// Create new blocks
while (b > 0) {
while (b) {
idx = b_end - --b;
item = array[idx];
key = is_computed_key ? keys[idx] : item;
Expand All @@ -417,9 +415,8 @@ function reconcile_tracked_array(
insert_each_item_block(block, dom, is_controlled, null);
}
} else {
var is_animated = (flags & EACH_IS_ANIMATED) !== 0;
var should_update_block =
(flags & (EACH_ITEM_REACTIVE | EACH_INDEX_REACTIVE)) !== 0 || is_animated;
var is_animated = !!(flags & EACH_IS_ANIMATED);
var should_update_block = flags & (EACH_ITEM_REACTIVE | EACH_INDEX_REACTIVE) || is_animated;
var start = 0;

/** @type {null | Text | Element | Comment} */
Expand Down Expand Up @@ -469,7 +466,7 @@ function reconcile_tracked_array(
} else if (start > b_end) {
b = start;
do {
if ((block = a_blocks[b++]) !== null) {
if ((block = a_blocks[b++])) {
destroy_each_item_block(block, active_transitions, apply_transitions);
}
} while (b <= a_end);
Expand All @@ -493,7 +490,7 @@ function reconcile_tracked_array(
pos = pos < a ? a : MOVED_BLOCK;
sources[a - start] = b;
b_blocks[a] = block;
} else if (block !== null) {
} else if (block) {
destroy_each_item_block(block, active_transitions, apply_transitions);
}
}
Expand All @@ -505,7 +502,7 @@ function reconcile_tracked_array(
var should_create;
if (is_animated) {
var i = b_length;
while (i-- > 0) {
while (i--) {
b_end = i + start;
a = sources[i];
if (pos === MOVED_BLOCK) {
Expand All @@ -517,7 +514,7 @@ function reconcile_tracked_array(
}
var last_block;
var last_sibling;
while (b_length-- > 0) {
while (b_length--) {
b_end = b_length + start;
a = sources[b_length];
should_create = a === -1;
Expand Down Expand Up @@ -600,7 +597,7 @@ function mark_lis(a) {
}

if (k < a[index[lo]]) {
if (lo > 0) {
if (lo) {
parent[i] = index[lo - 1];
}
index[lo] = i;
Expand Down Expand Up @@ -628,7 +625,7 @@ function mark_lis(a) {
function insert_each_item_block(block, dom, is_controlled, sibling) {
var current = /** @type {import('./types.js').TemplateNode} */ (block.d);

if (sibling === null) {
if (!sibling) {
if (is_controlled) {
return insert(current, /** @type {Element} */ (dom), null);
} else {
Expand Down Expand Up @@ -660,15 +657,15 @@ function get_first_child(block) {
function destroy_active_transition_blocks(active_transitions) {
var length = active_transitions.length;

if (length > 0) {
if (length) {
var i = 0;
var block;
var transition;

for (; i < length; i++) {
block = active_transitions[i];
transition = block.r;
if (transition !== null) {
if (!transition) {
block.r = null;
destroy_each_item_block(block, null, false);
}
Expand Down Expand Up @@ -705,16 +702,16 @@ export function get_first_element(block) {
* @returns {void}
*/
function update_each_item_block(block, item, index, type) {
if ((type & EACH_ITEM_REACTIVE) !== 0) {
if (type & EACH_ITEM_REACTIVE) {
set_signal_value(block.v, item);
} else if (is_lazy_property(block.v)) {
block.v.o[block.v.p] = item;
}
const transitions = block.s;
const index_is_reactive = (type & EACH_INDEX_REACTIVE) !== 0;
const index_is_reactive = !!(type & EACH_INDEX_REACTIVE);
// Handle each item animations
const each_animation = block.a;
if (transitions !== null && (type & EACH_KEYED) !== 0 && each_animation !== null) {
if (transitions && type & EACH_KEYED && each_animation) {
each_animation(block, transitions, index, index_is_reactive);
}
if (index_is_reactive) {
Expand All @@ -739,25 +736,25 @@ export function destroy_each_item_block(
) {
const transitions = block.s;

if (apply_transitions && transitions !== null) {
if (apply_transitions && transitions) {
// We might have pending key transitions, if so remove them first
for (let other of transitions) {
if (other.r === 'key') {
transitions.delete(other);
}
}
if (transitions.size === 0) {
if (!transitions.size) {
block.s = null;
} else {
trigger_transitions(transitions, 'out');
if (transition_block !== null) {
if (transition_block) {
transition_block.push(block);
}
return;
}
}
const dom = block.d;
if (!controlled && dom !== null) {
if (!controlled && dom) {
remove(dom);
}
destroy_signal(/** @type {import('./types.js').EffectSignal} */ (block.e));
Expand All @@ -775,15 +772,15 @@ export function destroy_each_item_block(
* @returns {import('./types.js').EachItemBlock}
*/
function each_item_block(item, key, index, render_fn, flags) {
const each_item_not_reactive = (flags & EACH_ITEM_REACTIVE) === 0;
const each_item_not_reactive = !(flags & EACH_ITEM_REACTIVE);

const item_value = each_item_not_reactive
? item
: (flags & EACH_IS_IMMUTABLE) === 0
: !(flags & EACH_IS_IMMUTABLE)
? mutable_source(item)
: source(item);

const index_value = (flags & EACH_INDEX_REACTIVE) === 0 ? index : source(index);
const index_value = !(flags & EACH_INDEX_REACTIVE) ? index : source(index);
const block = create_each_item_block(item_value, index_value, key);

const effect = render_effect(
Expand Down
6 changes: 3 additions & 3 deletions packages/svelte/src/internal/client/hydration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export function get_hydration_fragment(node) {

/** @type {null | string} */
let target_depth = null;
while (current_node !== null) {
while (current_node) {
const node_type = current_node.nodeType;
const next_sibling = current_node.nextSibling;
if (node_type === 8) {
Expand Down Expand Up @@ -61,7 +61,7 @@ export function get_hydration_fragment(node) {
export function hydrate_block_anchor(anchor_node, is_controlled) {
/** @type {Node} */
let target_node = anchor_node;
if (current_hydration_fragment !== null) {
if (current_hydration_fragment) {
if (is_controlled) {
target_node = /** @type {Node} */ (target_node.firstChild);
}
Expand All @@ -79,7 +79,7 @@ export function hydrate_block_anchor(anchor_node, is_controlled) {
set_current_hydration_fragment(fragment);
} else {
const first_child = /** @type {Element | null} */ (target_node.firstChild);
set_current_hydration_fragment(first_child === null ? [] : [first_child]);
set_current_hydration_fragment(!first_child ? [] : [first_child]);
}
}
}
4 changes: 2 additions & 2 deletions packages/svelte/src/internal/client/loop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ function run_tasks(now) {
task.f();
}
});
if (tasks.size !== 0) raf.tick(run_tasks);
if (tasks.size) raf.tick(run_tasks);
}

/**
Expand All @@ -33,7 +33,7 @@ export function clear_loops() {
export function loop(callback) {
/** @type {import('./private.js').TaskEntry} */
let task;
if (tasks.size === 0) raf.tick(run_tasks);
if (!tasks.size) raf.tick(run_tasks);
return {
promise: new Promise((fulfill) => {
tasks.add((task = { c: callback, f: fulfill }));
Expand Down
Loading