statically compress webpack artifacts #6266

akosyakov · 2019-09-26T01:44:22Z

What it does

fix [Webpack] Assets could be compressed to improve delivery times #4285: enable compressions of Theia build artifacts, i.e. bundles and monaco scripts, styles, wasm and so on.
cache does not work in watch mode webpack-contrib/compression-webpack-plugin#133 - in dev mode compressed artifacts are not reused, which leads to redownloading of all bundles on each refresh
- We have to decide whether we disable it for now in dev mode. If we do when for the initial load we have to pull 22.5 mb instead of 4.8mb. If we don't then during consequent incremental development we have to pull around 2Mb each time instead of only rebuilt bundles as before, e.g. for a change in the plugin ext it is 0.8Mb. cc @svenefftinge @AlexTugarev
check license compatibility of new dependencies

Production mode

before 9.3Mb is transferred:

after 2.5Mb is transferred:

Development mode

before 22.5Mb is transferred:

after 4.8Mb is transferred:

How to test

Build in production and development before and after changes.
Load the application without the cache and check how much traffic is transferred in each time.
In development mode pay attention whether bundles are reused on incremental change in extensions.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

yarn.lock

benoitf · 2019-09-27T08:51:15Z

I would not use it for dev mode.

akosyakov · 2019-10-10T17:05:27Z

I would not use it for dev mode.

Yeah, in such state I agree, but generally i would like to have it for the dev mode. I had bad experience of developing Theia from airports WiFis, Russian on mobile connections and obviously with EclipseCon WiFi in Ludwisburg. Looking into fixing compression with watch mode in the webpack plugin.

akosyakov · 2019-10-14T14:52:32Z

I've opened a PR against the compression plugin to improve the development mode: webpack-contrib/compression-webpack-plugin#134

For time being, I've published our fork with a fix and used it in this PR.

Results are pretty good, now in the development mode a change in the plugin system requires redownloading 134 kb vs 800kb before.

And initially one has to download 4.6 Mb for development instead of 22.5 before.

akosyakov · 2019-10-14T14:56:44Z

@AlexTugarev could you review, confirm results please? It would be good to have it done before EclipseCon.

akosyakov · 2019-10-14T15:18:34Z

I will add an option to pass to theia webpack to disable it.

akosyakov · 2019-10-15T07:55:12Z

Clarifications how to test:

disable cache in network tab
clean network tab and logs
reload the page
check transfered data in the network tab
test this PR and build from master to see the difference

AlexTugarev · 2019-10-15T08:01:45Z

how I verified this:

regular build in dev mode

this PR

vs current master

watched browser example in dev mode

(after changing core package)

this PR

(reloaded twice! divide in half for comparison)
(here I also played with link conditioner app on osx; switched to 3G mode)

vs current master

benoitf · 2019-10-15T08:04:02Z

we won't merge it until it PR is merged upstream, correct ?

akosyakov · 2019-10-15T08:05:02Z

@AlexTugarev yes, it shows you that for this PR 4.6 Mb is transferred vs 23.9 Mb on master. You should also check in dev mode, i.e.

disable disable cache
watch example app
watch some extension
change some extension
check how much is loaded against master

akosyakov · 2019-10-15T08:06:01Z

we won't merge it until it PR is merged upstream, correct ?

Why? We tested that it works for us in different conditions. PR in upstream has tests as well. webpack guys can review it for several months, EclipesCon is next week. There would be also an option to disable it if something goes wrong for downstream projects.

benoitf · 2019-10-15T08:15:17Z

But if we do forks on each issue we had on upstream projects it's like nightmare to maintain.
I don't see why we should use code from un-merged PR upstream.

EclipesCon is next wekk.

About EclipseCon, I don't see how it's related. If you work locally you don't transfer new stuff each time. If it's about hosted version, it's not related to Eclipse Theia but more a theia/gitpod assembly issue.

AlexTugarev · 2019-10-15T08:24:16Z

If it's about hosted version, it's not related to Eclipse Theia but more a theia/gitpod assembly issue.

@benoitf we want to make a Theia workshop, and this PR will improve the development experience a lot.

akosyakov · 2019-10-15T08:25:55Z

About EclipseCon, I don't see how it's related. If you work locally you don't transfer new stuff each time. If it's about hosted version, it's not related to Eclipse Theia but more a theia/gitpod assembly issue.

Yes, we are going to use Gitpod, but it is not related to how we deploy Gitpod. It is about improving development of Theia remotely. Gitpod, or any other services, should not do any caching/compression of user traffic (resources served by user apps). I've added to dev meeting. It is important for good experience next week. cc @marcdumais-work @svenefftinge

akosyakov · 2019-10-15T08:46:04Z

But if we do forks on each issue we had on upstream projects it's like nightmare to maintain.
I don't see why we should use code from un-merged PR upstream.

It is not ideal, but sometimes you should move forward somehow. We do it for @theia/node-pty and @typefox/monaco-editor-core as well.

benoitf · 2019-10-15T08:57:58Z

well, IMHO we shouldn't

For example node-pty upstream is 0.8.1, forked is still on 0.7.8
and monaco editor core is 0.18.1 while fork is 0.18.0-next.1 (so you don't know well the diff as well)

published package are also referencing to the upstream github URLs and not the forked one.

also I don't see why sometimes we use @theia namespace and sometimes @typefox

akosyakov · 2019-10-15T09:04:00Z

For Monaco there is no another way. We want to consume internal APIs of VS Code editor to implement VS Code support. Default Monaco distribution tree shakes a lot making it impossible. You can learn a bit more here: https://github.com/eclipse-theia/theia/wiki/LSP-and-Monaco-integration

For other, I'm agree that we should move to latest version at some point. But It's decided by a need of the project, not by desire to use latest versions. Otherwise we will spend all our time updating dependencies each day. If there are no important bug fixes and improvements in these dependencies, we can live with our forks forever there is no a tragedy here.

akosyakov · 2019-10-15T09:05:50Z

also I don't see why sometimes we use @theia namespace and sometimes @typefox

That's more the historical reason for Monaco. I will republish compression plugin under @theia.

akosyakov · 2019-10-15T09:25:57Z

published package are also referencing to the upstream github URLs and not the forked one.

I think it is good. Bugs should be opened against the parent repo. That's for example what devs do who forked monaco-languageclient last year. They send users to the main repo. It's also not really important since only we use it.

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov · 2019-10-15T10:48:47Z

tests should be fixed
one can pass --no-static-compression to theia build or theia watch to disable it
- the record added to CHANGELOG
the fork moved from my account under theia-org: https://github.com/theia-ide/compression-webpack-plugin/tree/theia_publishing
- the package is published under @theia npm scope as well

@AlexTugarev have you managed to reproduce perf improvements in dev mode?

akosyakov · 2019-10-15T14:38:55Z

We've agreed during the dev meeting with @marcdumais-work :

that this PR improves production experience generally, especially for our docker images;
it improves development mode on slow network;
that using our fork is fine whenever we find is practical, although:
- our solution should be properly communicated to the upstream project as an issue or a PR;
- fork should be under theia-ide org, under @theia npm scope name;
- there should be an issue to switch to upstream when an issue gets resolve there.

We can transfer monaco-editor-core under theia-ide org during next migration: #6223 (comment) Getting rid of Monaco fork is impossible, since it makes the plugin system impossible to implement.

@AlexTugarev Could you finish the review please?

AlexTugarev · 2019-10-16T05:57:57Z

dev-packages/application-manager/src/generator/webpack-generator.ts

    description: "Mode to use",
    choices: ["development", "production"],
    default: "production"
+}).option('static-compression', {


q: no-static-compression vs static-compression?

https://github.com/yargs/yargs/blob/master/docs/tricks.md#negate

AlexTugarev

This changes speed up iterating on changes in dev mode significantly. 👍

akosyakov requested review from svenefftinge, AlexTugarev, paul-marechal and vince-fugnitto September 26, 2019 01:44

akosyakov commented Sep 26, 2019

View reviewed changes

yarn.lock Show resolved Hide resolved

akosyakov mentioned this pull request Sep 26, 2019

GH-4285: Enforce Webpack Compression to improve serving times #4286

Closed

akosyakov added the performance issues related to performance label Sep 26, 2019

akosyakov force-pushed the ak/webpack_compress branch from ea481a7 to 1429aa1 Compare October 14, 2019 14:48

akosyakov marked this pull request as ready for review October 14, 2019 14:53

akosyakov force-pushed the ak/webpack_compress branch from 1429aa1 to ab046ba Compare October 14, 2019 15:03

akosyakov force-pushed the ak/webpack_compress branch 2 times, most recently from deafdb8 to fc34766 Compare October 15, 2019 10:35

fix #4285: statically compress webpack artifacts

df42f06

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/webpack_compress branch from fc34766 to df42f06 Compare October 15, 2019 10:46

AlexTugarev reviewed Oct 16, 2019

View reviewed changes

AlexTugarev approved these changes Oct 16, 2019

View reviewed changes

akosyakov merged commit 3ddb313 into master Oct 16, 2019

akosyakov deleted the ak/webpack_compress branch October 16, 2019 06:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statically compress webpack artifacts #6266

statically compress webpack artifacts #6266

akosyakov commented Sep 26, 2019 •

edited

Loading

benoitf commented Sep 27, 2019

akosyakov commented Oct 10, 2019

akosyakov commented Oct 14, 2019 •

edited

Loading

akosyakov commented Oct 14, 2019

akosyakov commented Oct 14, 2019

akosyakov commented Oct 15, 2019

AlexTugarev commented Oct 15, 2019 •

edited

Loading

benoitf commented Oct 15, 2019

akosyakov commented Oct 15, 2019

akosyakov commented Oct 15, 2019 •

edited

Loading

benoitf commented Oct 15, 2019

AlexTugarev commented Oct 15, 2019

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

benoitf commented Oct 15, 2019

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019

akosyakov commented Oct 15, 2019

AlexTugarev Oct 16, 2019

AlexTugarev Oct 16, 2019

AlexTugarev left a comment

statically compress webpack artifacts #6266

statically compress webpack artifacts #6266

Conversation

akosyakov commented Sep 26, 2019 • edited Loading

What it does

Production mode

Development mode

How to test

Review checklist

Reminder for reviewers

benoitf commented Sep 27, 2019

akosyakov commented Oct 10, 2019

akosyakov commented Oct 14, 2019 • edited Loading

akosyakov commented Oct 14, 2019

akosyakov commented Oct 14, 2019

akosyakov commented Oct 15, 2019

AlexTugarev commented Oct 15, 2019 • edited Loading

regular build in dev mode

this PR

vs current master

watched browser example in dev mode

this PR

vs current master

benoitf commented Oct 15, 2019

akosyakov commented Oct 15, 2019

akosyakov commented Oct 15, 2019 • edited Loading

benoitf commented Oct 15, 2019

AlexTugarev commented Oct 15, 2019

akosyakov commented Oct 15, 2019 • edited Loading

akosyakov commented Oct 15, 2019 • edited Loading

benoitf commented Oct 15, 2019

akosyakov commented Oct 15, 2019 • edited Loading

akosyakov commented Oct 15, 2019 • edited Loading

akosyakov commented Oct 15, 2019 • edited Loading

akosyakov commented Oct 15, 2019

akosyakov commented Oct 15, 2019

AlexTugarev Oct 16, 2019

Choose a reason for hiding this comment

AlexTugarev Oct 16, 2019

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

akosyakov commented Sep 26, 2019 •

edited

Loading

akosyakov commented Oct 14, 2019 •

edited

Loading

AlexTugarev commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading

akosyakov commented Oct 15, 2019 •

edited

Loading