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
perf(platform-browser): only append style element on creation #52237
Conversation
95cde75
to
8d62024
Compare
Prior to this change the style element was appended to host element multiple times. Whilst the actual element was not added multiple to the DOM multiple times. This causes a performance regression and it caused repainting. This can be observed in the below benchmark. ```js (() => { const time = (name, fn) => { const t = performance.now(); fn(); console.log(name, performance.now() - t); } const s = document.createElement("style"); s.textContent = "@layer x{} @font-face { font-family: foo; }"; time("append and enable", () => { document.head.append(s); s.disabled = false; }); time("compute body color", () => { getComputedStyle(document.body).color; }); time("compute body layout", () => { document.body.offsetTop; }); time("append and disable", () => { document.head.append(s); s.disabled = false; }); time("compute body color", () => { getComputedStyle(document.body).color; }); time("compute body layout", () => { document.body.offsetTop; }); })(); ``` Output ``` append and enable 0.20000000298023224 compute body color 0.7999999970197678 compute body layout 2.899999998509884 append and disable 0.10000000149011612 compute body color 0.7000000029802322 compute body layout 2.2999999970197678 ``` When commenting the 2nd `document.head.append(s);`, the results are slightly different and we can see that calling `getComputedStyle` does not incur any performance impact this is a result of no repainting. ``` append and enable 0.10000000149011612 compute body color 0.7999999970197678 compute body layout 3.1999999955296516 append and disable 0.10000000149011612 compute body color 0 compute body layout 0 ``` Pantheon benchmarks: http://docs/spreadsheets/d/1iLRLGCmVYZHuVRdI7dO_WM7wnQ1DvkS-tJzi-0-u1KY?resourcekey=0-kwtrf0nbAhcPqAGdqbdz4g#gid=0
8d62024
to
4a63ed2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It generally LGTM without knowing more about code, but I think we should add a test verifying that tags are added only once - otherwise it is way too easy to regress.
Calling You can run the below in the browser console to see this behaviour. [...document.head.children].forEach(n => n.remove());
const style = document.createElement('style');
console.log(document.head.children.length); // 0
document.head.appendChild(style);
document.head.appendChild(style);
console.log(document.head.children.length); // 1 |
This PR was merged into the repository by commit 7466004. |
Prior to this change the style element was appended to host element multiple times. Whilst the actual element was not added multiple to the DOM multiple times. This causes a performance regression and it caused repainting. This can be observed in the below benchmark. ```js (() => { const time = (name, fn) => { const t = performance.now(); fn(); console.log(name, performance.now() - t); } const s = document.createElement("style"); s.textContent = "@layer x{} @font-face { font-family: foo; }"; time("append and enable", () => { document.head.append(s); s.disabled = false; }); time("compute body color", () => { getComputedStyle(document.body).color; }); time("compute body layout", () => { document.body.offsetTop; }); time("append and disable", () => { document.head.append(s); s.disabled = false; }); time("compute body color", () => { getComputedStyle(document.body).color; }); time("compute body layout", () => { document.body.offsetTop; }); })(); ``` Output ``` append and enable 0.20000000298023224 compute body color 0.7999999970197678 compute body layout 2.899999998509884 append and disable 0.10000000149011612 compute body color 0.7000000029802322 compute body layout 2.2999999970197678 ``` When commenting the 2nd `document.head.append(s);`, the results are slightly different and we can see that calling `getComputedStyle` does not incur any performance impact this is a result of no repainting. ``` append and enable 0.10000000149011612 compute body color 0.7999999970197678 compute body layout 3.1999999955296516 append and disable 0.10000000149011612 compute body color 0 compute body layout 0 ``` Pantheon benchmarks: http://docs/spreadsheets/d/1iLRLGCmVYZHuVRdI7dO_WM7wnQ1DvkS-tJzi-0-u1KY?resourcekey=0-kwtrf0nbAhcPqAGdqbdz4g#gid=0 PR Close #52237
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…r#52237) Prior to this change the style element was appended to host element multiple times. Whilst the actual element was not added multiple to the DOM multiple times. This causes a performance regression and it caused repainting. This can be observed in the below benchmark. ```js (() => { const time = (name, fn) => { const t = performance.now(); fn(); console.log(name, performance.now() - t); } const s = document.createElement("style"); s.textContent = "@layer x{} @font-face { font-family: foo; }"; time("append and enable", () => { document.head.append(s); s.disabled = false; }); time("compute body color", () => { getComputedStyle(document.body).color; }); time("compute body layout", () => { document.body.offsetTop; }); time("append and disable", () => { document.head.append(s); s.disabled = false; }); time("compute body color", () => { getComputedStyle(document.body).color; }); time("compute body layout", () => { document.body.offsetTop; }); })(); ``` Output ``` append and enable 0.20000000298023224 compute body color 0.7999999970197678 compute body layout 2.899999998509884 append and disable 0.10000000149011612 compute body color 0.7000000029802322 compute body layout 2.2999999970197678 ``` When commenting the 2nd `document.head.append(s);`, the results are slightly different and we can see that calling `getComputedStyle` does not incur any performance impact this is a result of no repainting. ``` append and enable 0.10000000149011612 compute body color 0.7999999970197678 compute body layout 3.1999999955296516 append and disable 0.10000000149011612 compute body color 0 compute body layout 0 ``` Pantheon benchmarks: http://docs/spreadsheets/d/1iLRLGCmVYZHuVRdI7dO_WM7wnQ1DvkS-tJzi-0-u1KY?resourcekey=0-kwtrf0nbAhcPqAGdqbdz4g#gid=0 PR Close angular#52237
Prior to this change the style element was appended to host element multiple times. Whilst the actual element was not added multiple to the DOM multiple times. This causes a performance regression and it caused repainting.
This can be observed in the below benchmark.
Output
When commenting the 2nd
document.head.append(s);
, the results are slightly different and we can see that callinggetComputedStyle
does not incur any performance impact this is a result of no repainting.Pantheon benchmarks: http://docs/spreadsheets/d/1iLRLGCmVYZHuVRdI7dO_WM7wnQ1DvkS-tJzi-0-u1KY?resourcekey=0-kwtrf0nbAhcPqAGdqbdz4g#gid=0