Skip to content

Performance Budget Strategy #2918

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

Closed
jbalsas opened this issue Feb 11, 2020 · 21 comments
Closed

Performance Budget Strategy #2918

jbalsas opened this issue Feb 11, 2020 · 21 comments
Labels
rfc Similar to the RFC that are used in React.js, Ember.js, Gatsby.js and Rust, but to mark problems tha

Comments

@jbalsas
Copy link
Contributor

jbalsas commented Feb 11, 2020

Just an RFC for now to explore what we want to do regarding Perf. Budgets. As we keep adding more and more components to the mix, it's important that we look at how that affects our final output so we keep the impact of using our library is contained.

We currently have 2 main deliverables in Clay:

  • @clayui/css: It's "statically" added through a css link but accounts to most of the css we load nowadays. Need to keep it in check and try to trim it down if possible
  • @clayui components: Mostly loaded on demand, but need to check how it performs on aggregate when most of the components are used.

Some random literature bits:

@jbalsas jbalsas added the rfc Similar to the RFC that are used in React.js, Ember.js, Gatsby.js and Rust, but to mark problems tha label Feb 11, 2020
@bryceosterhaus
Copy link
Member

I read through the articles you mentioned and they all seemed to speak of a performance budget in regards to a specific webapp or site. Since Clay is a library, I am wondering what the best way to measure performance is since we can't really use something like lighthouse. Perhaps we could do something similar to material-ui, where they have a CI that analyzes the bundle size on each PR. example

@pat270
Copy link
Member

pat270 commented Feb 25, 2020

@jbalsas is there a specific size for @clayui/css we are trying to target?

@matuzalemsteles
Copy link
Member

I think trying to impose a performance budget inside Clay can be a bit much for a library, it would be difficult as Bryce said to measure the size of our packages. We do not deliver our processed packages with minification, dead code removal or more refined optimizations. I think this part is really up to the application to add these steps, since they can apply it in the best way.

Perhaps what we can follow here are package rules for the components, despite the fact that we are using very few dependencies that actually go into distribution. Perhaps we can apply some budget rules related to the component load time or component interaction. I still think that this would be very superficial and maybe it doesn't really help to measure since in an application context this may be better to measure.

For CSS, perhaps it makes sense to start imposing rules for the size of the final bundle, for example: Start with the current size of the bundle and impose a code reduction policy, decreasing a percentage of the bundle from time to time, so we can measure it. We can use some CI's to monitor this.

@wincent
Copy link
Contributor

wincent commented Feb 27, 2020

it would be difficult as Bryce said to measure the size of our packages

I am surprised that you would classify this as "difficult"; how hard can it really be? And doesn't it seem like something worthwhile even if it requires a little effort?

I've been looking at using Lighthouse to measure performance in liferay-portal and it looks pretty straightforward to gather some basic data even in a complex environment like a running portal instance; I would think that something as straightforward as measuring the size of a bundle would be dramatically simpler.

I think trying to impose a performance budget inside Clay can be a bit much for a library,

I think pretty much the opposite of this. Because Clay is a library, it consists of a very well-defined, isolated set of artifacts that can be measured. And because it is very hard for us to remove weight once added, measuring it systematically as an integral part of the development process seems like a desirable practice.

@matuzalemsteles
Copy link
Member

I am surprised that you would classify this as "difficult"; how hard can it really be? And doesn't it seem like something worthwhile even if it requires a little effort?

Not that this is difficult, I think it would be much easier than in an application. But I don't see an advantage in measuring the size of the package in our components, this is why we didn't spoil them minified, so we would be measuring a value maybe higher than what would be expected, maybe we will make sense by measuring in that value but we will be seeing values ​​higher than it could be when they are minified and have the package dependencies. Even though we do a build process here to measure these values, we are still not guaranteeing these real values ​​for those who are consuming. Does that make sense to you?

I think pretty much the opposite of this. Because Clay is a library, it consists of a very well-defined, isolated set of artifacts that can be measured. And because it is very hard for us to remove weight once added, measuring it systematically as an integral part of the development process seems like a desirable practice.

Yes, as we have dependencies we can restrict or create policies for that, but I still don't see why to measure the size of our components, the vast majority of them are just pure components in React... what we would add would be just third-party dependencies.

@wincent
Copy link
Contributor

wincent commented Feb 27, 2020

Even though we do a build process here to measure these values, we are still not guaranteeing these real values ​​for those who are consuming. Does that make sense to you?

I understand the argument, but I don't think it is valid. If we want to prepare a minified build of each package for the purposes of comparison — specifically, to track per-package and overall size trends over time —  it would be straightforward to do so, and it simply wouldn't matter at all if our minifier ended up not exactly matching the minification that a Clay client might end up employing; an approximation would be totally sufficient for us to establish a trend-line. In any case, remember that the single most important Clay client is Liferay's own usage in DXP, and that's a set-up that we can replicate as closer as we wished, if we considered it necessary — I just don't think it is.

Yes, as we have dependencies we can restrict or create policies for that, but I still don't see why to measure the size of our components, the vast majority of them are just pure components in React...

Don't focus too heavily on just the React components — everything I'm saying here should apply to @clayui/css and any other resources that form part of Clay. I'm actually less concerned with the footprint of the components (because you only pay for what you use) than I am with tracking the size of our CSS, which tends to get baked into themes in a bulk fashion, and as I said early, once added, is practically never removed.

@julien
Copy link
Contributor

julien commented Feb 27, 2020

I think we could just try to use Lighthouse and Chrome's devtools until we find a way to "automate" this testing.

A while back I watched this https://www.udacity.com/course/browser-rendering-optimization--ud860 and it gave me good tips on how to measure performance.

If we had a way to test components invidually it would be a good start.

@matuzalemsteles
Copy link
Member

I understand the argument, but I don't think it is valid. If we want to prepare a minified build of each package for the purposes of comparison — specifically, to track per-package and overall size trends over time — it would be straightforward to do so, and it simply wouldn't matter at all if our minifier ended up not exactly matching the minification that a Clay client might end up employing; an approximation would be totally sufficient for us to establish a trend-line. In any case, remember that the single most important Clay client is Liferay's own usage in DXP, and that's a set-up that we can replicate as closer as we wished, if we considered it necessary — I just don't think it is.

Hm yeah, makes sense. 🤔

Don't focus too heavily on just the React components — everything I'm saying here should apply to @clayui/css and any other resources that form part of Clay. I'm actually less concerned with the footprint of the components (because you only pay for what you use) than I am with tracking the size of our CSS, which tends to get baked into themes in a bulk fashion, and as I said early, once added, is practically never removed.

Yeah I agree with you, I'm thinking that our main focus should be on @clayui/css, we could have a policy to reduce the bundle...

@pat270
Copy link
Member

pat270 commented Feb 27, 2020

I'm actually less concerned with the footprint of the components (because you only pay for what you use) than I am with tracking the size of our CSS, which tends to get baked into themes in a bulk fashion, and as I said early, once added, is practically never removed.

Although we don't have a way to magically exclude unused CSS, it's possible, in the theme, to prevent specific @clayui/css components from being output to the final css file by creating /css/clay/_components.scss, then copy/paste contents of https://github.com/liferay/liferay-portal/blob/master/modules/apps/frontend-theme/frontend-theme-styled/src/main/resources/META-INF/resources/_styled/css/clay/_components.scss. You can delete any of the imports you don't need.

I've also created a way to prevent specific component styles from being output via variables. For example including the code below in _clay_variables.scss:

$management-bar-primary: (
    enabled: false,
);

will prevent any styles declared in .management-bar-primary from being output.

@bryceosterhaus
Copy link
Member

bryceosterhaus commented Feb 27, 2020

it would be difficult as Bryce said to measure the size of our packages

Oh I was actually trying to say the opposite. I think we should measure the size of our packages per PR.

prepare a minified build of each package for the purposes of comparison — specifically, to track per-package and overall size trends over time

Totally agree with this and I think it would be an easy next step. And especially if we bundle dependencies as well, we will have a realistic glimpse of how large it is for the client.

I'm also just going to create a ticket for this. #2965

@jbalsas
Copy link
Contributor Author

jbalsas commented Mar 2, 2020

Although we don't have a way to magically exclude unused CSS, it's possible, in the theme, to prevent specific @clayui/css components from being output to the final css file by creating /css/clay/_components.scss, then copy/paste contents of https://github.com/liferay/liferay-portal/blob/master/modules/apps/frontend-theme/frontend-theme-styled/src/main/resources/META-INF/resources/_styled/css/clay/_components.scss. You can delete any of the imports you don't need.

This is easier said than done... it's very difficult, if not nearly impossible to do this from a theme. It requires an absurd level of knowledge on what we provide and what's used in general where a theme applies.

While it's definitely a valid tool, this can't be our to-go answer in the first place.

@pat270
Copy link
Member

pat270 commented Mar 2, 2020

I'm unsure of what direction you're leaning towards regarding reducing the size of @clayui/clay-css. For @clayui/[email protected] as a whole, we can cut a bit from Bootstrap's source. We can cut any styles we overwrite in Clay CSS, since Bootstrap isn't designed to be fully configured via Sass variables.

We can remove auto generating breakpoint styles for sm, md, lg, xl for large components like navbar, management-bar, and navigation-bar. The only problem I see with removing auto generated breakpoint styles is breaking themes since we don't know who is using them.

On the Portal side of things we can include everything for the Admin theme, and include only a subset for Styled/Classic.

@jbalsas
Copy link
Contributor Author

jbalsas commented Mar 3, 2020

Hey @pat270, for now, I'd be happy if we start monitoring the size and try to keep it contained.

Once we're aware of what's going on with it we can start thinking about strategies to reduce it.

@bryceosterhaus
Copy link
Member

Does the current build stats satisfy this issue for now? We have a 5% threshold currently and show the size of all packages, including both base and atlas theme of clay-css.

@jbalsas
Copy link
Contributor Author

jbalsas commented Apr 28, 2020

Hey @bryceosterhaus, before closing this, I'd say we need to at least set an upper boundary. We could easily double our current size 4.99% at a time :)

Could you maybe collect a summary of current sizes and make an upper-boundary proposal to track it? Maybe a 5-10% of our current baseline. My main goal with this would be to give us some pause if/when we hit that limit so we can re-evaluate our strategy and react accordingly.

@bryceosterhaus
Copy link
Member

Yeah that makes sense, I'll look into that and also look into adding a way to track the growth over time. Possibly a file that gets updated once a week with build size, we could automate it with gh.

@bryceosterhaus
Copy link
Member

@jbalsas how to you feel about our current implementation for this? Currently we have a CI check for total size of packages(excluding clay-charts), https://github.com/liferay/clay/blob/master/package.json#L4.

Anything additional you would like to see for this issue?

@bryceosterhaus
Copy link
Member

fyi, 150kb was completely arbitrary, we can lower if if you see fit. This is the most recent size check https://github.com/liferay/clay/pull/3213/checks?check_run_id=664368855#step:9:27

@jbalsas
Copy link
Contributor Author

jbalsas commented May 12, 2020

Hey @bryceosterhaus, current implementation LGTM as a start! Nice job! 👏

fyi, 150kb was completely arbitrary, we can lower if if you see fit

Whatever number I said right now would also be completely arbitrary 😂. My suggestion to continue with this would be:

  • Let's say I'd like to lower that boundary to 125KB and then 100KB.
  • Right now we have something around 25% of leeway with the current size and limit... which seems like a safe enough cushion to work with. That is, we should lower the size respecting that margin, so we should try to see if we can get the library down to a 95KB and then down to 75KB (my math might be off, but you get the idea...)

So, what I'd like to see:

  • Create a research task to analyze the bundle and identify improvement opportunities
  • For each detected opportunity, let's create a task and when completed, reduce the max size respecting that 25% free-space "rule".

@bryceosterhaus
Copy link
Member

Updated how we run our checks in #3223

I lowered the threshold to 140kb since the build was 119kb and adding the new layout component will likely raise it over 125kb.

Create a research task to analyze the bundle and identify improvement opportunities

#3222

@bryceosterhaus
Copy link
Member

Now that we have size-limit and recently reduced our overall size by removing moment, I think we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Similar to the RFC that are used in React.js, Ember.js, Gatsby.js and Rust, but to mark problems tha
Projects
None yet
Development

No branches or pull requests

6 participants