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.

Sign up for GitHub

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

Jump to bottom

gwtOnLoad is assigned twice in a single statement #10060

Open
niloc132 opened this issue Dec 14, 2024 · 1 comment · May be fixed by #10061
Open

gwtOnLoad is assigned twice in a single statement #10060

niloc132 opened this issue Dec 14, 2024 · 1 comment · May be fixed by #10061

Comments

@niloc132
Copy link
Member

**GWT version: 2.7 to 2.12.1
Browser (with version): any
Operating System: any

Description

Discovered while exploring how compiled GWT modules start up, in looking for some ways to simplify this output slightly.

The current implementation generates code that assigns gwtOnLoad to itself, from the current init method. From the showcase JS in 2.12.1:

var gwtOnLoad = gwtOnLoad = sDb;

This is the comment in latest that describes what this block of code is supposed to do:

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java

Lines 2129 to 2137 in 339188d

/**
* <pre>
* var $entry = Impl.registerEntry();
* var gwtOnLoad = ModuleUtils.gwtOnLoad();
* {MODULE_RuntimeRebindRegistrator}.register();
* {MODULE_PropertyProviderRegistrator}.register();
* ModuleUtils.addInitFunctions(init1, init2,...)
* </pre>
*/

and the actual relevant code a few lines down:

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java

Lines 2146 to 2151 in 339188d

// var gwtOnLoad = ModuleUtils.gwtOnLoad;
JsName gwtOnLoad = topScope.findExistingUnobfuscatableName("gwtOnLoad");
JsVar varGwtOnLoad = new JsVar(sourceInfo, gwtOnLoad);
varGwtOnLoad.setInitExpr(createAssignment(gwtOnLoad.makeRef(sourceInfo),
indexedFunctions.get("ModuleUtils.gwtOnLoad").getName().makeRef(sourceInfo)));
globalStmts.add(new JsVars(sourceInfo, varGwtOnLoad));

The init expression for the JsVar should not itself contain an assignment, unless there is some benefit to doing so - and in this case, I can't imagine any, but I might not be imaginative enough...

Reviewing git history, an earlier commit (that was never released had this code, with the apparent expectation of allowing linkers to rewrite gwtOnLoad:

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java

Lines 1957 to 1971 in ec75b53

* // Stub gwtOnLoad at top level so that HtmlUnit can find it.
* // On first execution this will assign a value of null into gwtOnLoad.
* // It's value will actually be built inside of the following anonymous
* // function and subsequent executions will preserve the existing value.
* // Since gwtOnLoad is being varred, this will work in the global scope
* // as well as in speculative future scopes in which linkers might
* // choose to place the output.
* var gwtOnLoad = typeof gwtOnLoad === 'undefined' ? null : gwtOnLoad;
* // Create gwtOnLoad in function scope so the previousGwtOnLoad reference is preserved.
* (function() {
* var previousGwtOnLoad = gwtOnLoad;
* gwtOnLoad = function(errFn, modName, modBase, softPermutationId) {
* if (previousGwtOnLoad) {
* previousGwtOnLoad(errFn, modName, modBase, softPermutationId);
* }

As that was removed before it was even released, I'm inclined to ignore it as a fluke. The implementation that follows appears to do what it claims to do, but does use createAssignment to do so - perhaps in 339188d it was accidentally inlined instead of rewritten?

Next relevant change in history is simpler still, and dates to the beginning of of GWT's public history:
https://github.com/gwtproject/gwt/blame/ab0aa686820319e15d3adf123f4502dc3349f6de/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#L948-L969
No var or assignment at all.

Looking more closely at the code review for the change that introduced this https://gwt-review.googlesource.com/c/gwt/+/7821, there is some commentary modular compilation and the need to re-assign the variable, plus some bug in HtmlUnit - but the reassignment shouldn't need to bother the a = a = b pattern, which will unconditionally assign b to a, rather than somehow chain or redefine it. I'll do some more testing to be sure that nothing subtle is happening here in HtmlUnit or incremental compile. Note: I'm not 100% confident that "modular compile" is an earlier name for "incremental compile", but signs do point in that direction.

I'm inclined to think that this is just a bug introduced by accidentally reusing too much of the previous revision, and we can safely remove the extra assignment, and shrink every GWT app by about 10 bytes.
@niloc132
Copy link
Member Author

Note: I'm not 100% confident that "modular compile" is an earlier name for "incremental compile", but signs do point in that direction.

I'm less confident in that the more I look - https://gwt-review.googlesource.com/c/gwt/+/5295/7/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java shows an added param name and a TODO, but https://gwt-review.googlesource.com/c/gwt/+/12730 seems to remove that and imply from the commit message that the effort to be more "modular" can be cleaned up and removed. Looking at my own commit 63a0336 as a reaction to fc1baba, the term seems likely to be overloaded. Since gwt-user still has cycles, i have to conclude that this modularization didn't end up mattering enough to actually finish removing all of them?

References to incremental compilation predate all of the above (see a5b133e, unfortunately the review is a casualty of python updates within google...), though that is old enough that it might not reference the same idea of a feature - ab4f599 points out that what we now know as "incremental compilation" was previously "per file compilation".

niloc132 added a commit to niloc132/gwt that referenced this issue Dec 15, 2024
@niloc132
Remove extra assignment to gwtOnLoad
d0bcc81 
Only saves about 10 bytes per GWT application, but the extra assignment
appears to serve no purpose.

Fixes gwtproject#10060
@niloc132 niloc132 linked a pull request Dec 15, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove extra assignment to gwtOnLoad niloc132/gwt
1 participant
@niloc132