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

Concerns about loading ES modules #228

Open
PolariTOON opened this issue Jan 27, 2022 · 1 comment
Open

Concerns about loading ES modules #228

PolariTOON opened this issue Jan 27, 2022 · 1 comment

Comments

@PolariTOON
Copy link
Collaborator

Issue #216 is about refactoring the projet to use modern JavaScript features. PR #227 was the first part towards this goal, by introducing the first ES modules. We started considering the impact on performance so i'm pasting some previous comments to move the discussion here.

@PolariTOON wrote in #219:

I agree splitting the tests into several files, possibly ES modules, would be useful to make maintainance easier.
This would also heavily increase the number of requests made to load all the tests, but also to incrementally load them by importing them dynamically without waiting for the whole testbase to be fetched and parsed.
So the impact on performance have to be considered.

@PolariTOON wrote in #227:

I understand re-exporting all the specs from one place (tests.js) is convenient for legibility but i think this is far from being ideal from a performance point of view, because this means these 105 (!) imports are actually discovered by the browser after two consecutive subrequests: csstest.js, and then tests.js.

The easier way i see to improve that is to import the specs directly from csstest.js.
Or we could preload some (all?) the files with <link rel="modulepreload">, but the support for it is not great currently.

@LeaVerou wrote in #227:

Importing the same module twice does not fire a separate HTTP request. Also, isn't the whole point of HTTP/2 that multiple HTTP requests are fine? 😄

@SebastianZ wrote in #227:

tests.js is just one additional request, so it doesn't make much of a change to remove it. I could remove it, though I kept it for readablility and to keep the Specs structure unchanged.
[...]
I agree we should add <link rel="modulepreload">s. In browsers that don't support it it's just ignored, so there's no harm in adding it. I'd add one for all tests except the CSS 2 ones because they are excluded by default.
[...]
I've tested that now in Chrome 97 and Firefox 96 and didn't see any performance gain. Therefore I do not add them. @PolariTOON If you think we can somehow get a performance improvement by preloading resources, I suggest you open a separate issue and we'll discuss it there.


Let me reply to some of these comments:

Importing the same module twice does not fire a separate HTTP request.

This is not about importing the same module twice, but about the depth of the request graph which now looks like this:

index.html
└── csstest.js
    └── tests.js
        └── tests/a.js
        └── tests/b.js
        └── ...

This means that the browser have to wait for csstest.js to be fetched and parsed to fetch tests.js, and then wait again for it to be parsed to fetch all the modules under tests/. To allow the browser to load the page faster, we have to let it discover the files earlier. This can be achieved by reducing the depth of the module graph -- for example, importing all the modules from the top-level module (csstest.js currently) -- or even better (in theory), by declaring them in the HTML code via <link rel="modulepreload">.

Also, isn't the whole point of HTTP/2 that multiple HTTP requests are fine? 😄

Indeed, once the browser knows about the files under tests/, these ones can be fetched in parallel and it's fine with the help of HTTP/2.

I've tested that now in Chrome 97 and Firefox 96 and didn't see any performance gain.

As far as i know, only Blink browsers support module preloading currently so it's normal not to see any improvement in Firefox. As for Chrome, i guess the gain should only be visible if all the dependencies are preloaded. According to the waterfall below, this should save about 300ms, but this should not change the "Time taken" measurement on the page because it only counts the execution time and not the roundtrips between the client and the server.


Before #227:
css3test-classic-performance
After #227:
css3test-module-performance

@SebastianZ
Copy link
Collaborator

To allow the browser to load the page faster, we have to let it discover the files earlier. This can be achieved by reducing the depth of the module graph -- for example, importing all the modules from the top-level module (csstest.js currently)

Importing the modules directly from csstest.js and with that removing tests.js would be ok for me.

Also, isn't the whole point of HTTP/2 that multiple HTTP requests are fine? 😄

Indeed, once the browser knows about the files under tests/, these ones can be fetched in parallel and it's fine with the help of HTTP/2.

To clarify things, HTTP/2 is about reusing the same connection for multiple requests. Though multiple requests still cause some overhead due to their headers in comparison to a single one.

I've tested that now in Chrome 97 and Firefox 96 and didn't see any performance gain.

As far as i know, only Blink browsers support module preloading currently

Right, for anyone wanting to follow implementations in other browser engines, see https://wkb.ug/180574 and https://bugzil.la/1425310.

As for Chrome, i guess the gain should only be visible if all the dependencies are preloaded. According to the waterfall below, this should save about 300ms, but this should not change the "Time taken" measurement on the page because it only counts the execution time and not the roundtrips between the client and the server.

I'll try it again and provide some numbers.

Sebastian

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

2 participants