perf(hydration): avoid observer if element is in viewport #11639
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
@vue/compat
vue
commit: |
There was a problem hiding this comment.
I don't think this optimization is really necessary, since the amount components with lazy hydration will unlikely be a number that would lead to noticeable performance issues.
There's also no clear data showing the difference between the cost of ob.observe() vs. manually checking whether something is in viewport via getBoundingClientRect(). My intuition is that the difference is negligible.
Side note: the implementation has an issue where the return doesn't actually break the forEach look so it will call the hydrate multiple times - but again, I don't think this optimization is even needed in the first place.
|
Hey @yyx990803 , thank you for the review! I decided to benchmark both behaviors and it seems like there is an average of ~95% performance improvement when using bounding rect, which is far from negligible: I tested this in regular environments in the browser, below is a the benchmark code (if you'd like me to send the full test webpage, I will gladly do so): const target = document.getElementById('target');
function benchmarkIntersectionObserver() {
const start = performance.now();
const observer = new IntersectionObserver(entries => {
if (entries[0].isIntersecting) {
const duration = performance.now() - start;
console.log(`IntersectionObserver time: ${duration}ms`);
observer.disconnect();
}
});
observer.observe(target);
}
function benchmarkGetBoundingClientRect() {
const start = performance.now();
const {
top,
left,
bottom,
right
} = target.getBoundingClientRect()
const {
innerHeight,
innerWidth
} = window
if (
((top > 0 && top < innerHeight) || (bottom > 0 && bottom < innerHeight)) &&
((left > 0 && left < innerWidth) || (right > 0 && right < innerWidth))
) { // Given the test is for an in-viewport element, this is guaranteed to evaluate to true and can therefore be tested reliably
const duration = performance.now() - start;
console.log(`Bounding rect time: ${duration}ms`);
}
}
function runBenchmark() {
const intersectionObserverTime = benchmarkIntersectionObserver();
const boundingClientRectTime = benchmarkGetBoundingClientRect();
}
window.addEventListener('load', () => {
runBenchmark();
});From dozens of benchmarks, bounding rect consistently takes around 0.1ms, whereas the intersection observer consistently takes multiple milliseconds, sometimes even reaching 10 and higher, as seen in this screenshot: In terms of real world impact, it's not impossible for some websites to have numerous elements to cause a performance degradation, even if not quite noticeable for users (but still for crawlers, Lighthouse, and the likes). For example, having such intersection-based element for ads, content requiring scrolling, footer, links, etc, can easily reach even 10 elements in the same page, which can be a difference of around ~60ms load time if the observer is consistently on the faster end, and ~105ms load time if it's on the slower end. That calculation doesn't take into account developers who decide to make every element in the page intersection based, which despite being a bad practice because of the inherent performance decrease for above-the-fold content, shouldn't mean that the performance should be overly impacted, in which case the difference could even be a few hundreds milliseconds, depending on the complexity of the page, which would then be really noticable As for the error in implementation, I will gladly fix it. I haven't fully checked how the forEach operates (which I guess is my bad, and I apologize for it) so it's possible to refactor this into a boolean flag or a different method, if you deem it necessary. |
|
Hey @yyx990803 , apologies for the second mention. I just wanted to confirm that the lack of response to my follow-up means you still don't think this is necessary despite the major performance difference, and not that you haven't seen my follow-up? If you think a difference of 12ms is not enough to warrant an optimization, I'm perfectly fine with it. I'm just wondering whether or not you've seen my benchmarks. Thanks in advance! |
| if (elementIsVisibleInViewport(el)) { | ||
| hydrate() | ||
| ob.disconnect() | ||
| return () => {} |
There was a problem hiding this comment.
After looking at the benchmark I think this is worthwhile, but the implementation needs some adjustments.
Right now there is no way to break out of the forEach loop, returning an empty function here doesn't do anything. We need to refactor forEach so that the loop breaks if the callback returns false.
There was a problem hiding this comment.
I see. I'll look into refactoring the function, thank you!
There was a problem hiding this comment.
Is the proposed fix functional?
|
Merged into |
|
Sorry for the late response, gotten caught up with personal matters. Thanks for the help! |
This PR optimizes the intersection based hydration by avoiding the observer overhead if the element is visible initially, ensuring it is immediately hydrated, as opposed to waiting for the observer to run its callback
The approach is originally taken from https://github.com/nuxt/nuxt/pull/26468/files#diff-8e89e3233ee017161774c7e5df6b9a18cc926d5a1b4081e7fcbf9651109ce45d by @huang-julien