Reduce size of compiled code skipping unused helpers #2038
Conversation
Allowing magic comments to have complex values will make the good vehicles for method names and other stuff.
elia
added
compiler
runtime
compiled-size
| @@ -1,8 +1,8 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| module Opal::MagicComments | |||
| MAGIC_COMMENT_RE = /\A# *(\w+) *: *(\w+) *$/.freeze | |||
| EMACS_MAGIC_COMMENT_RE = /\A# *-\*- *(\w+) *: *(\w+) *-\*- *$/.freeze | |||
| MAGIC_COMMENT_RE = /\A# *(\w+) *: *(\S+.*?) *$/.freeze | |||
There was a problem hiding this comment.
ignore this comment if I'm overthinking, but the idea of using magic comments feels cryptic to me. Comments shouldn't be used to change behavior of your code. However Ruby does it, so I don't know if it's ok or not.
Also I think I'd personally expect these comments (if they are valuable at all) to be used as some kind of meta-information about the file (like by some external analyzer, rubocop is an example), and so I'd expect that I can safely remove them.
Is it that much worse writing var <helper> = Opal.<helper> at the beginning of the file?
There was a problem hiding this comment.
The way I see these helpers is that they're mostly for core stuff and very unlikely to be used elsewhere (although it wouldn't be a problem per se). Broadening the conversation to include also the use_strict comment I expect it to be transient and probably change it's default from false to true in v1.2. I personally dislike having to mark every file with frozen string literal although I like the effect.
In summary helpers should only (mostly?) live within opal core, and use_strict is transient. Hope in this perspective it becomes less of a concern.
| @@ -85,6 +85,7 @@ def compile_while | |||
| end | |||
|
|
|||
| def compile_iter | |||
| helper :slice | |||
There was a problem hiding this comment.
huh, just got the idea that shortcut is probably a better name
| add_temp '$$$ = Opal.const_get_qualified' | ||
| add_temp '$$ = Opal.const_get_relative' | ||
| add_temp '$$$ = Opal.$$$' | ||
| add_temp '$$ = Opal.$$' | ||
|
|
There was a problem hiding this comment.
This will make the newly generated code incompatible with the 1.0.0 runtime. I would suggest bumping the version to like 1.0.1.
There was a problem hiding this comment.
@ahmadine you're right, actually master is targeted toward 1.1, while the 1.0 branch is 1-0-stable and that will receive minor patches and bugfixes (actually I already released 1.0.2).
That said, it should still be compatible, as the local variable names won't change, except if someone was using stuff like Opal.$$.Regexp.$$.IGNORECASE.
I'll update master to be v1.1.0.alpha shortly
There was a problem hiding this comment.
I didn't mean "API" incompatibility, but "ABI" incompatibility. Otherwise, yeah, good to know, I missed the 1.0-stable branch.
There was a problem hiding this comment.
I see, yet, old compiled code should keep working (except for the Opal.$$.Regexp… example, which was only meant to aid debugging). Both Opal.const_get_relative and Opal.const_get_qualified are still available. Sorry if I ask again, it's because I want to be sure to really understand the implications of these changes 🙂
There was a problem hiding this comment.
New code uses $$ = Opal.$$ which, when combined with the old runtime, would be equal to $$ = undefined. And that isn't going to work, because Opal.$$ = Opal.const_get_qualified is just defined in this changeset.
I think most people wouldn't be affected, but if someone combined the prebuilt CDN "binaries" with the master code which says "1.0.0", he could be surprised.
There was a problem hiding this comment.
Oh I see now, and yes I agree it shouldn't happen and it will be even less likely when I change the dev version to 1.1.0.alpha 👍
Each file can load an internal helper without requiring it to be available for every single compiled file.
Previously those were preloaded for every file, this saves some bytes: BEFORE: development: 771.05KB, minified: 473.63KB, gzipped: 91.58KB AFTER: development: 769.25KB, minified: 472.60KB, gzipped: 91.53KB (via bundle exec rake dist FILES=opal)
elia
force-pushed
the
elia/helpers-compaction
branch
from
923526f
to
ab5df53
Compare
I attempted to make
nilinto an optional helper, but it's so ubiquitous that I eventually gave up.