Remove side effect exports #4261

jridgewell · 2016-07-28T22:37:13Z

It seems Closure compiler can't DCE (notice timer, platform, and log are still in the compiled code -- We expect them to be gone since they're never called -- exports that are a side-effect. Side effects exports are the result of some function call, either a CallExpression (normal function call) or a NewExpression (creating a new instance of a "class" function).

This is breaking the SW especially, since there are references to the window variable, which is undeclared inside the SW environment.

So, let's change these side-effects into runtime side-effects (vs static compile-time side-effects). That way, closure can properly DCE.

TODO:

@erwinmombay is nice enough to look into updating our dev.assert() optimization to work with the new dev().assert() syntax. After that, I'll be able to post size diffs.

dvoytenko · 2016-07-29T16:11:30Z

@jridgewell @erwinmombay Can we confirm that new form gets DCE'd?

@jridgewell timer.js will need to be refactored into timer-impl.js/timer.js pattern. Otherwise now only core's instance ever gets installed, but code is deployed in all extensions. It'd be fine if it's done in a separate PR, of course.

erwinmombay · 2016-07-29T16:36:30Z

test/size.txt

+ 710.5 kB   |   140.48 kB   |    44.05 kB   |   shadow-v0.js / amp-shadow.js
+    787 B   |       406 B   |       sw.js
+ 83.97 kB   |    19.54 kB   |   sw.max.js


can you add normalizeRow(rows, 'sw.js', 'sw.max.js', true);
here https://github.com/ampproject/amphtml/blob/master/build-system/tasks/size.js#L98

will fix this bug, currently it doesn't automatically relate these two files together since it isnt under extensions and our other top level files don't all follow the .max convention.

I reverted the changes.

erwinmombay · 2016-07-29T16:46:03Z

@dvoytenko @jridgewell confirmed the DCE is working with the compilerpass changes, all sizes are back or close to a 1kb +/- off from the one on master c69c82f

jridgewell · 2016-07-29T17:02:17Z

I think everything's within .1kb of what it was. Some things, like a4a, went down 2kb.

erwinmombay · 2016-07-29T22:04:55Z

src/timer.js

@@ -29,6 +29,9 @@ export class Timer {
   * @param {!Window} win
   */
  constructor(win) {
+    if (!win || !win.setTimeout) {
+      throw new Error('Fuck you');


The tests are really starting to get on my nerves.

jridgewell · 2016-08-02T21:40:54Z

timer.js will need to be refactored into timer-impl.js/timer.js pattern.

#4329

Side effects can’t be DCE’d.

jridgewell · 2016-08-03T21:40:11Z

PTAL

dvoytenko · 2016-08-03T22:32:51Z

LgTm

Fixes ampproject#4370. Re: ampproject#4261.

Fixes #4370. Re: #4261.

* Do not export a side effect Side effects can’t be DCE’d. * Fix side effect in Performance * Remove platform side effect * Remove Timer#now * eliminate dev().assert() and dev().fine() calls from src/log.js module * Fix timer side effect * Fix Log side effect * Fix tests - part 1 * These damn tests * Fix service worker tests * Fix Analytics tests * Linting * Fix error * Fix Signin * fixes * Disable test * Disable check-types * Fix merge

Fixes ampproject#4370. Re: ampproject#4261.

* Do not export a side effect Side effects can’t be DCE’d. * Fix side effect in Performance * Remove platform side effect * Remove Timer#now * eliminate dev().assert() and dev().fine() calls from src/log.js module * Fix timer side effect * Fix Log side effect * Fix tests - part 1 * These damn tests * Fix service worker tests * Fix Analytics tests * Linting * Fix error * Fix Signin * fixes * Disable test * Disable check-types * Fix merge

Fixes ampproject#4370. Re: ampproject#4261.

jridgewell added the NEEDS REVIEW label Jul 28, 2016

jridgewell assigned dvoytenko Jul 28, 2016

erwinmombay reviewed Jul 29, 2016
View reviewed changes

jridgewell force-pushed the side-effect-dce branch 2 times, most recently from be4a8ed to 3c5b11a Compare July 29, 2016 16:54

erwinmombay reviewed Jul 29, 2016
View reviewed changes

jridgewell force-pushed the side-effect-dce branch from 52880a7 to e03564d Compare August 2, 2016 21:27

jridgewell mentioned this pull request Aug 2, 2016

Separate Timer code from Timer Installation #4329

Closed

jridgewell force-pushed the side-effect-dce branch from 2fde34b to 025dd76 Compare August 3, 2016 20:15

erwinmombay and others added 16 commits August 3, 2016 17:12

eliminate dev().assert() and dev().fine() calls from src/log.js module

92b616a

Do not export a side effect

1789748

Side effects can’t be DCE’d.

Fix side effect in Performance

5c09626

Remove platform side effect

7f3950e

Remove Timer#now

a583200

Fix timer side effect

e3b8686

Fix Log side effect

f4bd9ee

Fix tests - part 1

80b3d46

These damn tests

74c5f7f

Fix service worker tests

5f29f77

Fix Analytics tests

577233b

Linting

81e023e

Fix error

6f59081

Fix Signin

2810efc

fixes

adbf71b

Disable test

52a49ff

jridgewell added 2 commits August 3, 2016 17:14

Disable check-types

c8763f1

Fix merge

76b80b8

jridgewell force-pushed the side-effect-dce branch from 025dd76 to 76b80b8 Compare August 3, 2016 21:14

This was referenced Aug 3, 2016

Amp Sidebar: Skipped Platform tests #4358

Closed

History: Skipped tests #4359

Closed

Double Click Ad Integration Test #4360

Closed

Analytics Visibility: Skipped Test #4361

Closed

dvoytenko added LGTM and removed NEEDS REVIEW labels Aug 3, 2016

jridgewell merged commit fe63400 into ampproject:master Aug 3, 2016

jridgewell deleted the side-effect-dce branch August 3, 2016 22:35

jridgewell added a commit to jridgewell/amphtml that referenced this pull request Aug 4, 2016

Fix intersection errors

857da70

Fixes ampproject#4370. Re: ampproject#4261.

jridgewell mentioned this pull request Aug 4, 2016

Fix intersection errors #4379

Merged

jridgewell added a commit that referenced this pull request Aug 4, 2016

Fix intersection errors (#4379)

17c3203

Fixes #4370. Re: #4261.

This was referenced Aug 15, 2016

Amp ooyala #4333

Closed

eliminate dev().assert() and dev().fine() calls from src/log.js module #4271

Closed

dreamofabear mentioned this pull request Aug 29, 2016

Re-enable History tests #4758

Merged

ariangibson pushed a commit to Mixpo/amphtml that referenced this pull request Sep 7, 2016

Fix intersection errors (ampproject#4379)

814d9d1

Fixes ampproject#4370. Re: ampproject#4261.

mityaha pushed a commit to ooyala/amphtml that referenced this pull request Nov 30, 2016

Fix intersection errors (ampproject#4379)

46d455b

Fixes ampproject#4370. Re: ampproject#4261.

cvializ mentioned this pull request Apr 21, 2017

Forbid creating maps at module-load time #8880

Closed

12 tasks

ampprojectbot mentioned this pull request Oct 30, 2021

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove side effect exports #4261

Remove side effect exports #4261

jridgewell commented Jul 28, 2016 •

edited

Loading

dvoytenko commented Jul 29, 2016

erwinmombay Jul 29, 2016

jridgewell Jul 29, 2016

erwinmombay commented Jul 29, 2016 •

edited

Loading

jridgewell commented Jul 29, 2016

erwinmombay Jul 29, 2016

jridgewell Jul 29, 2016

jridgewell commented Aug 2, 2016

jridgewell commented Aug 3, 2016

dvoytenko commented Aug 3, 2016

Remove side effect exports #4261

Remove side effect exports #4261

Conversation

jridgewell commented Jul 28, 2016 • edited Loading

TODO:

dvoytenko commented Jul 29, 2016

erwinmombay Jul 29, 2016

Choose a reason for hiding this comment

jridgewell Jul 29, 2016

Choose a reason for hiding this comment

erwinmombay commented Jul 29, 2016 • edited Loading

jridgewell commented Jul 29, 2016

erwinmombay Jul 29, 2016

Choose a reason for hiding this comment

jridgewell Jul 29, 2016

Choose a reason for hiding this comment

jridgewell commented Aug 2, 2016

jridgewell commented Aug 3, 2016

dvoytenko commented Aug 3, 2016

jridgewell commented Jul 28, 2016 •

edited

Loading

erwinmombay commented Jul 29, 2016 •

edited

Loading