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

Speed up the CKEditor 5 development environment #13643

Closed
pomek opened this issue Mar 8, 2023 · 6 comments · Fixed by #13871
Closed

Speed up the CKEditor 5 development environment #13643

pomek opened this issue Mar 8, 2023 · 6 comments · Fixed by #13871
Assignees
Labels
squad:devops Issue to be handled by the Devops team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@pomek
Copy link
Member

pomek commented Mar 8, 2023

📝 Provide a description of the improvement

Why

What / ideas

  • A new third thread could help. It could handle testing CKEditor 5 features.
  • Try to execute CKEditor 5 features in parallel. Still determining how many threads are at the same time, but for sure, we can handle two processes simultaneously.
    • AFAIR Travis gives a single thread, so we must make some magic with promises and the JavaScript loop.
  • Perhaps it's time to configure CircleCI to compare times.
  • (?)

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@pomek pomek added type:improvement This issue reports a possible enhancement of an existing feature. squad:devops Issue to be handled by the Devops team. labels Mar 8, 2023
@Reinmar
Copy link
Member

Reinmar commented Mar 9, 2023

  • AFAIR Travis gives a single thread, so we must make some magic with promises and the JavaScript loop.

That won't speed up anything (at least not significantly) if it's still a single thread. Running two Chrome instances may also be prone to these focus issues unless the headless Chrome can take care of detaching focus/selection from the OS in which it operates.

@Reinmar
Copy link
Member

Reinmar commented Mar 9, 2023

It's PITA that a quite simple change requires almost one hour to verify.

48 minutes for automated tests and 25 minutes for other validation tasks. This indicates that we should indeed focus now on optimizing how automated tests are executed.

Here, the main thing to blame is that every package tests are run completely separately. The reason for this is that we wanted to have separate coverage for each of them. But this means that  testing every package start everything from scratch – build everything (transpile TS, run webpack, instrument coverage), start the env (Karma, Chrome), finally: execute the tests.

It'd be great to know which part of that process take how much time. Whether this is the building or execution step (starting the env is most likely fairly cheap). So, the imporant question to answer: What's percentage cost of the build and execution steps for a typical package?

If building is costly: Look for options to speed this up. We saw already how e.g. using Vite can help. Any progress here would also benefit our dev environment, so it'd be awesome to have meaningful improvements here, even if the build step is <40% of total time. Some ideas:

  • Vite or other bundler?
  • Caching between webpack processes?
  • Prebuilding JS to TS once (similar to caching)?
  • Using single build for all packages (which would required figuring out the magic to still count coverage separately for each package)?

If the execution of tests is the major problem, then we have fewer options, unfortunately.

  • We might look for more powerful CI machines
  • or split this task into two jobs

@filipsobol
Copy link
Member

filipsobol commented Mar 9, 2023

I agree that using promises will not help us much, but will probably increase the complexity of our tools. In the current state of the JavaScript ecosystem, we have to run TypeScript (tsc, ts-loader, etc.) to check types or generate d.ts files. However, for code transpilation and bundling we should aim to update our tools to something that can handle TypeScript files, but ignores typing, such as esbuild (I used it in the past and liked it a lot), vite (same), or swc.

I did some quick tests with promising results, but it (obviously) requires much more research.

I ran the yarn test -f XXX --reporter=dots --production --coverage command with the current Karma + webpack configuration for 3 packages and these are the results:

Package nameTotal timewebpack timewebpack time as %
clipboard36s29s81%
engine71s37s52%
ui44s32s73%

Then I replaced the ts-loader with esbuild-loader and ran the same commands:

Package nameTotal timewebpack timewebpack time as %Improvement over ts-loader
clipboard10s4s40%-72%
engine42s10s19%-41%
ui17s5s29%-61%

Then I did the same with swc-loader and results were identical. The only difference was that esbuild-loader had some minor issues with code coverage.

With this configuration, the travis-check.js script took 14m10s locally (down from 31m47s).

In my experience, if we went even further and replaced karma-webpack with karma-esbuild, then the build time would be negligible (200-1000ms). Unfortunately, to my knowledge switching to `esbuild` isn't something we can do quickly, because we have a lot of tools written specifically for webpack.

What we can probably do in a reasonable amount of time is to prebuild all the TypeScript code before running the tests, so that Webpack doesn't have to handle it. There are some improvements we can make in this area as well.

I ran build commands for all packages with the current setup, then updated tsconfigs to use TypeScript's project references (see changes on my test branch), then changed the same configuration to only emit declarations (no JS output).

Current setup8m 02s
Project references2m 25s
Project references + emitDeclarationOnly: true2m 18s

Another small win might be the caching of yarn dependencies.

@pomek
Copy link
Member Author

pomek commented Mar 10, 2023

Thanks @filipsobol for the detailed research.

I am fine with replacing ts-loader with esbuild-loader. Then, we can talk about the next steps.

@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Mar 13, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Mar 13, 2023
@przemyslaw-zan
Copy link
Member

swc-loader seems to be quite problematic, as it causes many failed tests and coverage issues.

esbuild-loader caused less problems, as there are only minor gaps in code coverage, but the tests pass. Additionally, it is stricter than ts-loader, as case in which file exports a type but the index.ts of given package does not specify that export as a type, causes an error. [fix]

We need to find a solution to the broken coverage with the esbuild-loader. Swapping babel-loader to istanbul-instrumenter-loader does not help here.

@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. labels Mar 16, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Mar 30, 2023
@martnpaneq
Copy link
Contributor

Some progress was achieved with esbuild-loader and CI is green now.

  1. Most of the issues were caused by esbuild removing /* istanbul ignore */ comments when transpiling code from typescript to javascript. In order to keep them, -- @preserve option needs to be added to all of them.

  2. In the engine package, there were some new errors, but their origin is not fully known.
    Looks like some paths, like missing else statements, that were previously just an info icon, are turning into errors - only on CI.

Prev:
image
Now:
image

To fix those errors I used 4 new /* istanbul ignore */ comments.

  1. One /* istanbul ignore */ comment that was meant for a single case clause is not being preserved even with -- @preserve option (in this file).
    I tried writing a proper test for it, but it took too long time with no progress. I had to ignore the whole switch statement.

  2. The last thing to fix are CK_DEBUG comments. If we want them preserved, we also need to add -- @preserve flag to them. I added some code to the loader that handles them in ckeditor5-dev, but I need some help with testing it.

There are 2 PRs to merge:
ckeditor/ckeditor5-dev#862 - needs testing for CK_DEBUIG loader with -- @preserve option
#13809 - needs to wait for the PR above and need to add-- @preserve option to CK_DEBUG comments

@pomek pomek changed the title Speed up CKEditor 5 CI Speed up the CKEditor 5 development environment Apr 12, 2023
pomek added a commit that referenced this issue Apr 12, 2023
Internal: Added the `-- @preserve` annotation to code coverage instructions to preserve these comments while processing the code using `esbuild-loader`. See #13643.

Tests: Write a few additional tests to keep the 100% code coverage to enable migration to `esbuild-loader`.
pomek added a commit to ckeditor/ckeditor5-dev that referenced this issue Apr 13, 2023
Feature (utils): Exports a new object called `loaders` containing several methods for configuring webpack. Available helpers present as follow:

  * `getTypeScriptLoader()` – returns a configuration for processing TypeScript files using esbuild,
  * `getJavaScriptLoader()` – returns a configuration for enabling `CK_DEBUG` flags in JavaScript files,
  * `getStylesLoader()` – returns a configuration for processing CSS files using PostCSS,
  * `getIconsLoader()` – returns a configuration for loading SVG icons using `raw-loader`,
  * `getFormattedTextLoader()` – returns a configuration for loading rich text files using `raw-loader`,
  * `getCoverageLoader()` – returns a configuration that installs instruments for measuring the code coverage.

Other (tests): Use loader helpers exported by the `@ckeditor/ckeditor5-dev-utils` package to create webpack configurations for automated and manual tests.

Other (tests): The `source-map` option is enabled by default due to converting TypeScript into JavaScript.

MAJOR BREAKING CHANGE (tests): Replaced `ts-loader` with `esbuild-loader` in manual and automated tests to improve the build time. See ckeditor/ckeditor5#13643.
pomek added a commit that referenced this issue Apr 13, 2023
Other: Switched from `ts-loader` to `esbuild-loader` when processing the TypeScript sources. It significantly reduces the build time when processing automated and manual tests or preparing snippets in the documentation. Closes #13643.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Apr 13, 2023
@CKEditorBot CKEditorBot added this to the iteration 62 milestone Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:devops Issue to be handled by the Devops team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants