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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary requests from new notification.js build #6646

Closed
3 tasks
chikeichan opened this issue May 23, 2019 · 0 comments
Closed
3 tasks

Remove unnecessary requests from new notification.js build #6646

chikeichan opened this issue May 23, 2019 · 0 comments
Assignees
@chikeichan
Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. type-enhancement

Comments

@chikeichan
Copy link
Contributor

chikeichan commented May 23, 2019 •
edited
Loading

Call stack is for reference - it can be reproduced using chrome dev tool -> performance tab.

Just on first glance, injectCss, txHelper, buy-eth-url, first-time-flow, and home.container should be removable from notification.js, reducing load time by around 500ms. (MBP 2017, total load time was 2s).

This PR should:

  • Use chrome perf tools to identify long activities that are removable
  • remove them
  • QA all functionality of the notification window (confirm tx, confirm token send, confirm contract interactions, all the sign messages, connect to provider, unlock) to make sure that things still works. It should already be covered by existing unit test.

Screen Shot 2019-05-22 at 6 01 33 PM
@chikeichan chikeichan self-assigned this May 23, 2019
@chikeichan chikeichan mentioned this issue May 23, 2019
Merged
@danjm danjm added this to the UI Sprint 13 [May 27] milestone May 27, 2019
@chikeichan chikeichan mentioned this issue Jun 3, 2019
Closed
@bdresser bdresser removed this from the UI Sprint 13 [May 27] milestone Jul 8, 2019
Gudahtt added a commit to Gudahtt/metamask-extension that referenced this issue Jul 23, 2019
@Gudahtt
Serve CSS as an external file
6369f41 
The CSS is now served as an external file instead of being injected.
This was done to improve performance. Ideally we would come to a middle
ground between this and the former behaviour by injecting only the CSS
that was required for the initial page load, then lazily loading the
rest. However that change would be more complex. The hope was that
making all CSS external would at least be a slight improvement.

Performance metrics were collected before and after this change to
determine whether this change actually helped. The metrics collected
were the timing events provided by Chrome DevTools:

* DOM Content Loaded (DCL) [1]
* Load (L) [2]
* First Paint (FP) [3]
* First Contentful Paint (FCP) [3]
* First Meaningful Paint (FMP) [3]

Here are the results (units in milliseconds):

Injected CSS:

| Run | DCL | L | FP | FCP | FMP |
| :---: | ---: | ---: | ---: | ---: | ---: |
| 1 | 1569.45 | 1570.97 | 1700.36 | 1700.36 | 1700.36 |
| 2 | 1517.37 | 1518.84 | 1630.98 | 1630.98 | 1630.98 |
| 3 | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| 4 | 1522.15 | 1523.72 | 1629.3 | 1629.3 | 1629.3 |
| **Min** | 1517.37 | 1518.84 | 1629.3 | 1629.3 | 1629.3 |
| **Max** | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| **Mean** | 1553.17 | 1554.71 | 1668.3 | 1668.3 | 1668.3 |
| **Std. dev.** | 33.41 | 33.43 | 38.16 | 38.16 | 38.16 |

External CSS:

| Run | DCL | L | FP | FCP | FMP |
| :---: | ---: | ---: | ---: | ---: | ---: |
| 1 | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| 2 | 1537.55 | 1538.99 | 199.38 | 1633.5 | 1633.5 |
| 3 | 1571.28 | 1572.74 | 268.65 | 1677.03 | 1677.03 |
| 4 | 1510.98 | 1512.33 | 206.72 | 1607.03 | 1607.03 |
| **Min** | 1510.98 | 1512.33 | 199.38 | 1607.03 | 1607.03 |
| **Max** | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| **Mean** | 1553.8025 | 1555.7425 | 239.93 | 1657.605 | 1657.605 |
| **Std. dev.** | 29.5375 | 30.0825 | 36.88 | 37.34 | 37.34 |

Unfortunately, using an external CSS file made no discernible improvement
to the overall page load time. DCM and L were practically identical, and
FCP and FMP were marginally better (well within error margins).

However, the first paint time was _dramatically_ improved. This change
seems worthwhile for the first paint time improvement alone. It also
allows us to delete some code and remove a dependency.

The old `css.js` module included two third-party CSS files as well, so
those have been imported into the main Sass file. This was easier than
bundling them in the gulpfile.

The resulting CSS bundle needs to be served from the root because we're
using a few `@include` rules that make this assumption. We could move
this under `/css/` if desired, but we'd need to update each of these
`@include` rules.

Relates to MetaMask#6646

[1]: https://developer.mozilla.org/en-US/docs/Web/Events/DOMContentLoaded
[2]: https://developer.mozilla.org/en-US/docs/Web/Events/load
[3]: https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics
@Gudahtt Gudahtt mentioned this issue Jul 23, 2019
Merged
Gudahtt added a commit that referenced this issue Jul 23, 2019
@Gudahtt
Serve CSS as an external file
1df7f8a 
The CSS is now served as an external file instead of being injected.
This was done to improve performance. Ideally we would come to a middle
ground between this and the former behaviour by injecting only the CSS
that was required for the initial page load, then lazily loading the
rest. However that change would be more complex. The hope was that
making all CSS external would at least be a slight improvement.

Performance metrics were collected before and after this change to
determine whether this change actually helped. The metrics collected
were the timing events provided by Chrome DevTools:

* DOM Content Loaded (DCL) [1]
* Load (L) [2]
* First Paint (FP) [3]
* First Contentful Paint (FCP) [3]
* First Meaningful Paint (FMP) [3]

Here are the results (units in milliseconds):

Injected CSS:

| Run | DCL | L | FP | FCP | FMP |
| :--- | ---: | ---: | ---: | ---: | ---: |
| 1 | 1569.45 | 1570.97 | 1700.36 | 1700.36 | 1700.36 |
| 2 | 1517.37 | 1518.84 | 1630.98 | 1630.98 | 1630.98 |
| 3 | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| 4 | 1522.15 | 1523.72 | 1629.3 | 1629.3 | 1629.3 |
| **Min** | 1517.37 | 1518.84 | 1629.3 | 1629.3 | 1629.3 |
| **Max** | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| **Mean** | 1553.17 | 1554.71 | 1668.3 | 1668.3 | 1668.3 |
| **Std. dev.** | 33.41 | 33.43 | 38.16 | 38.16 | 38.16 |

External CSS:

| Run | DCL | L | FP | FCP | FMP |
| :--- | ---: | ---: | ---: | ---: | ---: |
| 1 | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| 2 | 1537.55 | 1538.99 | 199.38 | 1633.5 | 1633.5 |
| 3 | 1571.28 | 1572.74 | 268.65 | 1677.03 | 1677.03 |
| 4 | 1510.98 | 1512.33 | 206.72 | 1607.03 | 1607.03 |
| **Min** | 1510.98 | 1512.33 | 199.38 | 1607.03 | 1607.03 |
| **Max** | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| **Mean** | 1553.8025 | 1555.7425 | 239.93 | 1657.605 | 1657.605 |
| **Std. dev.** | 29.5375 | 30.0825 | 36.88 | 37.34 | 37.34 |

Unfortunately, using an external CSS file made no discernible improvement
to the overall page load time. DCM and L were practically identical, and
FCP and FMP were marginally better (well within error margins).

However, the first paint time was _dramatically_ improved. This change
seems worthwhile for the first paint time improvement alone. It also
allows us to delete some code and remove a dependency.

The old `css.js` module included two third-party CSS files as well, so
those have been imported into the main Sass file. This was easier than
bundling them in the gulpfile.

The resulting CSS bundle needs to be served from the root because we're
using a few `@include` rules that make this assumption. We could move
this under `/css/` if desired, but we'd need to update each of these
`@include` rules.

Relates to #6646

[1]: https://developer.mozilla.org/en-US/docs/Web/Events/DOMContentLoaded
[2]: https://developer.mozilla.org/en-US/docs/Web/Events/load
[3]: https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics
Gudahtt added a commit that referenced this issue Jul 23, 2019
@Gudahtt
Serve CSS as an external file (#6894)
0f8a9a5 
The CSS is now served as an external file instead of being injected.
This was done to improve performance. Ideally we would come to a middle
ground between this and the former behaviour by injecting only the CSS
that was required for the initial page load, then lazily loading the
rest. However that change would be more complex. The hope was that
making all CSS external would at least be a slight improvement.

Performance metrics were collected before and after this change to
determine whether this change actually helped. The metrics collected
were the timing events provided by Chrome DevTools:

* DOM Content Loaded (DCL) [1]
* Load (L) [2]
* First Paint (FP) [3]
* First Contentful Paint (FCP) [3]
* First Meaningful Paint (FMP) [3]

Here are the results (units in milliseconds):

Injected CSS:

| Run | DCL | L | FP | FCP | FMP |
| :--- | ---: | ---: | ---: | ---: | ---: |
| 1 | 1569.45 | 1570.97 | 1700.36 | 1700.36 | 1700.36 |
| 2 | 1517.37 | 1518.84 | 1630.98 | 1630.98 | 1630.98 |
| 3 | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| 4 | 1522.15 | 1523.72 | 1629.3 | 1629.3 | 1629.3 |
| **Min** | 1517.37 | 1518.84 | 1629.3 | 1629.3 | 1629.3 |
| **Max** | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| **Mean** | 1553.17 | 1554.71 | 1668.3 | 1668.3 | 1668.3 |
| **Std. dev.** | 33.41 | 33.43 | 38.16 | 38.16 | 38.16 |

External CSS:

| Run | DCL | L | FP | FCP | FMP |
| :--- | ---: | ---: | ---: | ---: | ---: |
| 1 | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| 2 | 1537.55 | 1538.99 | 199.38 | 1633.5 | 1633.5 |
| 3 | 1571.28 | 1572.74 | 268.65 | 1677.03 | 1677.03 |
| 4 | 1510.98 | 1512.33 | 206.72 | 1607.03 | 1607.03 |
| **Min** | 1510.98 | 1512.33 | 199.38 | 1607.03 | 1607.03 |
| **Max** | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| **Mean** | 1553.8025 | 1555.7425 | 239.93 | 1657.605 | 1657.605 |
| **Std. dev.** | 29.5375 | 30.0825 | 36.88 | 37.34 | 37.34 |

Unfortunately, using an external CSS file made no discernible improvement
to the overall page load time. DCM and L were practically identical, and
FCP and FMP were marginally better (well within error margins).

However, the first paint time was _dramatically_ improved. This change
seems worthwhile for the first paint time improvement alone. It also
allows us to delete some code and remove a dependency.

The old `css.js` module included two third-party CSS files as well, so
those have been imported into the main Sass file. This was easier than
bundling them in the gulpfile.

The resulting CSS bundle needs to be served from the root because we're
using a few `@include` rules that make this assumption. We could move
this under `/css/` if desired, but we'd need to update each of these
`@include` rules.

Relates to #6646

[1]: https://developer.mozilla.org/en-US/docs/Web/Events/DOMContentLoaded
[2]: https://developer.mozilla.org/en-US/docs/Web/Events/load
[3]: https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics
@danfinlay danfinlay added area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. type-enhancement labels Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chikeichan chikeichan

Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. type-enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Create separate javascript bundle for notification.html chikeichan/metamask-plugin
5 participants
@danfinlay @bdresser @danjm @chikeichan @hilvmason