Skip to content

Remove unused variable in output #690

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
Merged
merged 1 commit into from
Sep 6, 2017
Merged

Remove unused variable in output #690

merged 1 commit into from
Sep 6, 2017

Conversation

flimzy
Copy link
Member

@flimzy flimzy commented Sep 2, 2017 •
edited
Loading

TL;DR The $ptr variable in every output function appears never to be used, so this eliminates it. I expect it's an obsolete hold-over from some previous change.

The story:

I noticed while playing with uglifyjs, a ton of warnings such as:

WARN: Dropping unused variable $ptr [main.js:95,666198]
WARN: Dropping unused variable $ptr [main.js:95,667947]

Which piqued my curiosity, to see if GopherJS could possibly be made intelligent enough to eliminate this $ptr value from the output when it's not used.

Looking at the output of one of my compiled projects, I saw many examples such as:

        Object.ptr.prototype.Get = function(key) {
                var $ptr, key, o;
                o = this;
                return o.object[$externalize(key, $String)];
        };

Some grepping further revealed that the only time we ever set the variable is in lines such as:

                /* */ } return; } if ($f === undefined) { $f = { $blk: Cond.ptr.prototype.Wait }; } $f.$ptr = $ptr; $f._r = _r; $f.c = c; $f.$s = $s; $f.$r = $r; return $f;

which is just restoring the function state after returning from a possibly-blocking function call.

I found no other use of this variable, so I removed it, and re-compiled me project, both with and without the -m flag, and got the following size reductions:

Build command Original bytes sans $ptr bytes Savings (%)
gopherjs build 11,743,976 11,608,077 135,899 (1.16%)
gopherjs build -m 7,634,995 7,523,944 111,051 (1.45%)

My project appears to run perfectly with the modification; I look forward to the CI test results as well.

@flimzy
Copy link
Member Author

flimzy commented Sep 2, 2017

Hrm, one failure:

=== RUN   TestMaxExecDepth
FAIL	text/template	2.272s

This passes fine on my local machine. Is this possibly related to #687? (Apologies if the answer should be obvious--I've not been following that issue at all, except reading headlines)

@dmitshur
Copy link
Member

dmitshur commented Sep 2, 2017 •
edited
Loading

Thanks for this PR, this is very interesting!

Is this possibly related to #687?

Yes, definitely looks related.

Some grepping further revealed that the only time we ever set the variable is in lines such as:

/* */ } return; } if ($f === undefined) { $f = { $blk: Cond.ptr.prototype.Wait }; } $f.$ptr = $ptr; $f._r = _r; $f.c = c; $f.$s = $s; $f.$r = $r; return $f;

which is just restoring the function state after returning from a possibly-blocking function call.

Can you elaborate on what will happen after the variable is no longer generated?

I expect it's an obsolete hold-over from some previous change.

It would be nice to find the source commit where it's introduced, perhaps it'll help explain the original motivation.

@dmitshur dmitshur requested a review from neelance September 2, 2017 17:16
@flimzy
Copy link
Member Author

flimzy commented Sep 2, 2017 •
edited by dmitshur
Loading

which is just restoring the function state after returning from a possibly-blocking function call.

Can you elaborate on what will happen after the variable is no longer generated?

The line in question is constructed here. So by not adding $ptr to the c.localVars list, it is neither set nor read. Does this answer the question?

@flimzy
Copy link
Member Author

flimzy commented Sep 2, 2017 •
edited
Loading

It would be nice to find the source commit where it's introduced, perhaps it'll help explain the original motivation.

It appears it was added in df432fe, where it was referenced in compiler/expressions.go, but then a few days later with 923aee9, that reference was removed, but the original declaration was left behind.

@dmitshur
Copy link
Member

dmitshur commented Sep 6, 2017 •
edited
Loading

Now that #687 is merged, I'm eager to rebase this PR and try CI again.

dmitshur
dmitshur approved these changes Sep 6, 2017
View reviewed changes
Copy link
Member

@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing the answers in the last 2 comments @flimzy, that's very helpful.

I've rebased and CI passed this time.

Based on the rationale provided, this change LGTM.

Thanks a lot for finding it, investigating it, and sending the PR!

@dmitshur dmitshur merged commit c7ececa into gopherjs:master Sep 6, 2017
@flimzy flimzy deleted the unusedVar branch September 6, 2017 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitshur dmitshur dmitshur approved these changes

@neelance neelance Awaiting requested review from neelance

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@flimzy @dmitshur