Small improvements to build size #17
Conversation
|
Size Change: -220 B (-8.85%) ✅ Total Size: 2.27 kB
|
There was a problem hiding this comment.
@shaseley the second commit is relatively minor additional wins, so happy to revert any or all of it
| @@ -57,7 +57,7 @@ class PostMessageCallbackMananger { | |||
| * @param {function(): undefined} callback | |||
| * @return {number} A handle that can used for cancellation. | |||
| */ | |||
| queueCallback(callback) { | |||
| queueCallback_(callback) { | |||
There was a problem hiding this comment.
these two are taking the JSCompiler interpretation of private as file-level private, not class-level :) Doesn't seem like too much of a stretch?
There was a problem hiding this comment.
No objections here. IIRC the intent was for this to be internal, so minimizing impact of this sgtm.
| @@ -161,7 +161,7 @@ class HostCallback { | |||
| * Returns true iff this task was scheduled with MessageChannel. | |||
| * @return {boolean} | |||
| */ | |||
| isMessageChannelCallback() { | |||
| isMessageChannelCallback_() { | |||
There was a problem hiding this comment.
This is really just unused (except for tests), but happy to revert since the intention appears for it to be usable in scheduler, similar to isIdleCallback
There was a problem hiding this comment.
Yeah this was only needed for tests, and I vaguely recall being hesitant about adding the bytes. I believe this was added because some places this was used didn't have MessageChannel, and this was helpful for a regression test. But minimizing the impact of this LGTM.
| /** | ||
| * Returns an array containing the elements of this task queue in order. This | ||
| * is meant to be used for debugging and might be removed. | ||
| * | ||
| * @param {IntrusiveTaskQueue} queue | ||
| * @return {!Array<!Object>} | ||
| */ | ||
| function toArray(queue) { | ||
| let node = queue.head_; | ||
| const a = []; | ||
| while (node !== null) { | ||
| a.push(node); | ||
| node = node.tq_next_; | ||
| } | ||
| return a; | ||
| } | ||
|
|
||
| export {IntrusiveTaskQueue, toArray}; |
There was a problem hiding this comment.
Just a suggestion. It does seem handy for debugging, so maybe worth keeping around, but putting it on a separate export allows it to get tree shaken out of the distributed polyfill. OTOH, if you're debugging code using the distributed polyfill, this function won't be in there anymore.
There was a problem hiding this comment.
Good catch. TBH I can't remember the history here, and removing it would probably be fine (better?) since it's unused in code. But I like this.
| @@ -161,7 +161,7 @@ class HostCallback { | |||
| * Returns true iff this task was scheduled with MessageChannel. | |||
| * @return {boolean} | |||
| */ | |||
| isMessageChannelCallback() { | |||
| isMessageChannelCallback_() { | |||
There was a problem hiding this comment.
Yeah this was only needed for tests, and I vaguely recall being hesitant about adding the bytes. I believe this was added because some places this was used didn't have MessageChannel, and this was helpful for a regression test. But minimizing the impact of this LGTM.
| @@ -57,7 +57,7 @@ class PostMessageCallbackMananger { | |||
| * @param {function(): undefined} callback | |||
| * @return {number} A handle that can used for cancellation. | |||
| */ | |||
| queueCallback(callback) { | |||
| queueCallback_(callback) { | |||
There was a problem hiding this comment.
No objections here. IIRC the intent was for this to be internal, so minimizing impact of this sgtm.
| /** | ||
| * Returns an array containing the elements of this task queue in order. This | ||
| * is meant to be used for debugging and might be removed. | ||
| * | ||
| * @param {IntrusiveTaskQueue} queue | ||
| * @return {!Array<!Object>} | ||
| */ | ||
| function toArray(queue) { | ||
| let node = queue.head_; | ||
| const a = []; | ||
| while (node !== null) { | ||
| a.push(node); | ||
| node = node.tq_next_; | ||
| } | ||
| return a; | ||
| } | ||
|
|
||
| export {IntrusiveTaskQueue, toArray}; |
There was a problem hiding this comment.
Good catch. TBH I can't remember the history here, and removing it would probably be fine (better?) since it's unused in code. But I like this.
The main improvement (in d9766b8) is to turn on terser property-name mangling for any property with a leading underscore, since they're all intended to be class-private anyways, so doing this doesn't run into any of the issues with using terser for general property renaming.
This brings the polyfill from 7998 bytes (2170 bytes with brotli) to 6481 bytes (2004 bytes with brotli).
The second commit (9bd39db) is some optional code tweaks to better minify a few small parts. Together they bring down the polyfill size to 6341 bytes (1970 bytes with brotli). All in all, about 20% fewer raw bytes, 9% fewer brotli bytes, nice, though not the biggest deal since the brotlied size is already so small. We can brag about sub-2KB now, I guess :)
There are some additional changes that could be made, like making the scheduler-internal task properties underscored, but the mixture of new properties and properties taken from the
optionsfelt fiddly and it starting feeling like something a compiler should be doing, not a person, so I let it be.