-
Notifications
You must be signed in to change notification settings - Fork 49
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
RFC: Remove Build Time Transforms #117
Comments
If I remember correctly runtime performance was the overall goal. This was not only about reducing bundle size but also about not invoking a component at all. I remember some impressive numbers about the performance win but wasn't able to find the blog post anymore. I'm not sure if this is still a concern as invoking components might not that expensive anymore. |
You're right @jelhan, I had forgotten about that. Not sure if glimmer components will give us the same performance and I have no idea how to measure that. I'm happy to make that a goal, but we'd need a way to measure it first. |
@jrjohnson that's always been the problem with ember components, a ton of rendering overhead. A lot of small components (like a ton of icons!) is like death from a 1000x cuts. |
@arthur5005 any reliable benchmarking tools we can use to play with this idea? I'd like to know if
I strongly suspect from some conversations with our ember devs that because we're returning a node and not a string that there was never a substantial performance increase. I'm very open to amending this RFC to only transform static icons |
@jrjohnson we couldn't find any proper tools, so we decided to come up with a setup that illustrates the basic overhead of initial renders using the 3 types of components in Ember: Classic Ember, Glimmer Components and Template Only Glimmer Components. Did some benchmarking by rendering 10,000 components; all we were looking for was the component rendering overhead. We fed a uniq value to each component to render (1-10,000) to make sure we weren't triggering some optimization. On Ember 3.14 we compared:
We had a button to render each set, then a reset button to measure teardown of the components, the following flame chart from the browser shows the results: |
@arthur5005 I'm reading this as glimmer component setup / tear down is about 2x slower than a plan div but 2x faster than a classic Ember component. What I'm not sure about is how much this actually matters. If this is one second faster rendering 10K components does that mean its 1ms faster for 10? |
I believe so. I'm no glimmer rendering expert, but this stuff adds up quickly. We have a page with about 120 cards, and in each of the cards there's a component that is just a simple badge. Each badge (using an Ember component) has an overhead of about 0.5ms. Your knee jerk is "like yeah whatever", but if the average card has 7 badges 120 x 7 = 840 badges Half a second is pretty big. Switching to glimmer cuts that in half for us, using just a div cuts that down further. There's other techniques that help, like occlusion rendering, pagination etc, but they can be costly in dev time or can have quirks or drawbacks. Every bit of rendering time helps and allows us more options. |
I'm surprised that there isn't a difference between Glimmer Component with and without a baking JavaScript class. I thought a template only glimmer component should be way faster cause not requiring to create a component instance. 😕 |
@jelhan the overhead almost has to be the same; something has to represent the component one way or the other. As far as I know, the net gains by moving to glimmer components was achieved by abandoning a lot of the callable surface area and lifecycle hooks of an Ember component; a template only component has the same surface area and lifecycle hooks because their functional ability is exactly the same. |
FWIW the main advantage of a template-only component might be that it puts less memory pressure on the garbage collector because the backing class is shared between all of those components. just a guess though... 😉 |
Would it make implementation easier if there was a separate component that would generate transformed output, like I don't use the current transform as I don't want all icons to be transformed as that would result in excessive duplication of the SVG data. For the two icons I do want transformed I just copied the generated SVG into the template.hbs directly and that works just fine. Personally I'd be keen for the feature to be removed if it meant more time could be spent on other parts of the plugin. I'd love Duotone support :) |
Nice @elgordino, one option I've been thinking about is only transforming completely static icons and an explicit component to signal that would possibly work very well. This would combine very well with transforming |
Ah, love this solution! Easy way to opt into sprites. |
Would appreciate any feedback from you all on the addition of |
It is currently possible to transform icons at build time from components into plain HTML using the option
enableExperimentalBuildTimeTransform
. This option was taken from the community addon and intended to accomplish two goals:Reduce application filesize
In order to support dynamic options
<FaIcon @icon="pencil" spin={{isSpinning}} />
we were never able to ship plain strings which the template compiler could de-duplicate. In FontAwesome v5 with it's use of inline SVGs these transformed nodes are actually very large so application file size could be larger when build time transforms were enabled.Remove unused icons
It is not possible in the current ember build system to detect what icons are used in templates and include only those icons in the build. While the build time transform does allow some icons to be detected it still requires developers to include dynamic icons into the
environment.js
file and was never reliable across addon boundaries.Conclusion
As the
enableExperimentalBuildTimeTransform
adds significant complexity to this addon and may be impossible to port to the new ember embroiderer addon build and as the main goals of this option were never accomplished I propose removing it entirely before av1.0.0
is released.The text was updated successfully, but these errors were encountered: