Skip to content
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

Open
jrjohnson opened this issue Sep 8, 2019 · 14 comments
Open

RFC: Remove Build Time Transforms #117

jrjohnson opened this issue Sep 8, 2019 · 14 comments

Comments

@jrjohnson
Copy link
Collaborator

jrjohnson commented Sep 8, 2019

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:

  1. Reduce file size of the application
  2. Remove unused icons from the build

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 a v1.0.0 is released.

@jelhan
Copy link
Contributor

jelhan commented Nov 8, 2019

This option was taken from the community addon and intended to accomplish two goals:

  1. Reduce file size of the application
  2. Remove unused icons from the build

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.

@jrjohnson
Copy link
Collaborator Author

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.

@arthur5005
Copy link

@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.

@jrjohnson
Copy link
Collaborator Author

@arthur5005 any reliable benchmarking tools we can use to play with this idea? I'd like to know if

  1. Glimmer Components are any better
  2. if our current transform actually results in a decrease

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 <FaIcon @icon='pencil' spin={{true}} /> as they are substantially less complicated. Whereas most of the annoyance in supporting these comes from something like <FaIcon @icon={{this.icon}} spin={{this.isSpinning}} />. Before doing that however, I'd like to be able to measure the actual performance impact.

@arthur5005
Copy link

arthur5005 commented Dec 19, 2019

@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:

  • Rendering 10,000 Ember Components
  • Rendering 10,000 Glimmer Components
  • Rendering 10,000 Template Only Glimmer Components
  • Rendering 10,000 <div>s with the value, instead of calling a component.

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:
Screen Shot 2019-12-19 at 9 53 13 AM

@jrjohnson
Copy link
Collaborator Author

@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?

@arthur5005
Copy link

arthur5005 commented Dec 19, 2019

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
then add the 120 cards
# of components = 840 + 120 = 960
total component overhead: 960 * 0.5 = 480ms

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.

@jelhan
Copy link
Contributor

jelhan commented Dec 20, 2019

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. 😕

@arthur5005
Copy link

arthur5005 commented Jan 15, 2020

@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.

@Turbo87
Copy link

Turbo87 commented Feb 7, 2020

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... 😉

@elgordino
Copy link

elgordino commented Feb 22, 2020

Would it make implementation easier if there was a separate component that would generate transformed output, like <FaIconInlineSVG> or something? This component could be more explicit in not taking dynamic options.

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 :)

@jrjohnson
Copy link
Collaborator Author

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 <FaStaticIcon> into using a sprite instead of the fairly lengthy SVG string. This should speed up rendering significantly in places where many of the same icon are being used without blowing up the template size at the same time.

@arthur5005
Copy link

Ah, love this solution! Easy way to opt into sprites.

@jrjohnson
Copy link
Collaborator Author

Would appreciate any feedback from you all on the addition of <FaStaticSprite> in #138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants