Remove side effect exports#4261
Conversation
|
@jridgewell @erwinmombay Can we confirm that new form gets DCE'd? @jridgewell |
test/size.txt
Outdated
| 58.5 kB | 5.42 kB | 2.37 kB | alp.js / alp.max.js | ||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I reverted the changes.
|
@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 |
be4a8ed to
3c5b11a
Compare
|
I think everything's within .1kb of what it was. Some things, like a4a, went down 2kb. |
src/timer.js
Outdated
There was a problem hiding this comment.
The tests are really starting to get on my nerves.
52880a7 to
e03564d
Compare
|
2fde34b to
025dd76
Compare
Side effects can’t be DCE’d.
025dd76 to
76b80b8
Compare
|
PTAL |
|
LgTm |
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
* 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
It seems Closure compiler can't DCE (notice
timer,platform, andlogare 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 aCallExpression(normal function call) or aNewExpression(creating a new instance of a "class" function).This is breaking the SW especially, since there are references to the
windowvariable, 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 newdev().assert()syntax. After that, I'll be able to post size diffs.