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

🏗Extract and indirect log message string #22146

Merged
merged 105 commits into from Jul 23, 2019

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented May 6, 2019

In progress.

Picks up from #19870

Reduces binary size by creating a table of log messages to be served from a URL.

v0 size

singlepass

gulp dist --single_pass --esm && compress -kc dist/v0.js | wc -c

with compress as brotli or gzip:

brotli gzip
before 67046 81463
after 65277 79178

~2.8% reduction.

multipass

Removed from PR.

Issues with strings not indirected

  • ActionService.error_ is used with variable messages (6). We could add a helper method to Log to specify strings that should be indirected.

build-system/build.conf.js Outdated Show resolved Hide resolved
build-system/build.conf.js Outdated Show resolved Hide resolved
@alanorozco alanorozco force-pushed the extract-logs branch 2 times, most recently from 01998c9 to 7759d83 Compare May 24, 2019 05:39
@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #22146 into master will decrease coverage by 44.61%.
The diff coverage is 37.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #22146       +/-   ##
===========================================
- Coverage   85.63%   41.02%   -44.62%     
===========================================
  Files        1556      237     -1319     
  Lines      146115    20491   -125624     
===========================================
- Hits       125130     8407   -116723     
+ Misses      20985    12084     -8901
Flag Coverage Δ
#integration_tests 41.02% <37.14%> (-0.01%) ⬇️
#unit_tests ?
Impacted Files Coverage Δ
src/service/url-replacements-impl.js 3.51% <ø> (-87.69%) ⬇️
src/service/crypto-impl.js 4.54% <0%> (-86.57%) ⬇️
src/custom-element.js 11.94% <0%> (-81.34%) ⬇️
src/cookies.js 68.29% <100%> (-29.39%) ⬇️
src/log.js 61.08% <37.93%> (-36.1%) ⬇️
extensions/amp-form/0.1/form-submit-service.js 0% <0%> (-100%) ⬇️
src/exponential-backoff.js 0% <0%> (-100%) ⬇️
src/utils/math.js 0% <0%> (-100%) ⬇️
src/service/viewer-cid-api.js 0% <0%> (-100%) ⬇️
...nsions/amp-viewer-integration/0.1/focus-handler.js 0% <0%> (-100%) ⬇️
... and 1467 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31657d1...5d91c91. Read the comment docs.

@alanorozco alanorozco merged commit 569d208 into ampproject:master Jul 23, 2019
@alanorozco alanorozco deleted the extract-logs branch July 23, 2019 16:59
rindo pushed a commit to logly/amphtml that referenced this pull request Jul 24, 2019
Reduces binary size by creating a table of log messages to be served from a URL (~2.8% bundle size reduction).

This change does NOT turn on message replacement, only extraction. Next release is planned to turn replacements on.
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
Reduces binary size by creating a table of log messages to be served from a URL (~2.8% bundle size reduction).

This change does NOT turn on message replacement, only extraction. Next release is planned to turn replacements on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants