-
Notifications
You must be signed in to change notification settings - Fork 399
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: optimize scoped ID for light/native #4399
Conversation
vmBeingRendered | ||
); | ||
} | ||
} |
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.
I got rid of these logError
s because they seemed pretty low-value (only logged in dev mode, should arguably be the job of accessibility tools like aXe, not LWC), and I couldn't think of a good way to maintain the dev-only logging when the optimization is in place.
@@ -135,6 +135,7 @@ jobs: | |||
- run: API_VERSION=61 yarn sauce:ci | |||
- run: API_VERSION=61 DISABLE_SYNTHETIC=1 yarn sauce:ci | |||
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 yarn sauce:ci | |||
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 DISABLE_STATIC_CONTENT_OPTIMIZATION=1 yarn sauce:ci |
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.
Just another case I think is worth testing, especially if we start actually shipping disableSyntheticShadowSupport=true
.
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.
Eagerly anticipating another chore(ci): rebalance karma tests
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.
You guessed it! It's imbalanced again. I wish GH had an easier way to do this. 🫠
@@ -135,6 +135,7 @@ jobs: | |||
- run: API_VERSION=61 yarn sauce:ci | |||
- run: API_VERSION=61 DISABLE_SYNTHETIC=1 yarn sauce:ci | |||
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 yarn sauce:ci | |||
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 DISABLE_STATIC_CONTENT_OPTIMIZATION=1 yarn sauce:ci |
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.
Eagerly anticipating another chore(ci): rebalance karma tests
Details
Fixes #3658.
This avoids rendering the
genScopedId
function (akagid
aka global ID) orscopedFragId
(akafid
aka fragment ID) for light DOM templates or shadow DOM templates whendisableSyntheticShadowSupport
is true. These functions only do anything in synthetic shadow DOM mode:lwc/packages/@lwc/engine-core/src/framework/api.ts
Lines 580 to 582 in 84e0536
lwc/packages/@lwc/engine-core/src/framework/api.ts
Lines 606 to 608 in 84e0536
None of our existing benchmarks are able to suss out the perf improvement. I did write a benchmark (nolanlawson@1599b49) which demonstrates the improvement, but I felt it was too tailored to this PR to make sense to commit to
master
. Here is the improvement (6-9%):Here is a before-and-after to show what the compiled template looks like before and after this change. Note that on the right, everything is moved into static HTML fragments:
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
Since the
gid
/fid
functions only do anything in synthetic shadow DOM, disabling them for light DOM or native shadow should be un-observable.