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.

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

dart2js: Importing unittest disables tree shaking #12862

Closed
rakudrama opened this issue Aug 28, 2013 · 15 comments
Closed

dart2js: Importing unittest disables tree shaking #12862

rakudrama opened this issue Aug 28, 2013 · 15 comments
Labels
closed-cannot-reproduce Closed as we were unable to reproduce the reported issue web-dart2js
Milestone

Comments

@rakudrama
Copy link
Member

It seems unfortunate and perhaps unintended that simply using unittest disables such an important optimization.

To repro, patch in https://codereview.chromium.org/23653003/
and run
$ tools/test.py -mrelease -cdart2js -rd8 dart2js_extra/tree_woe_test

@peter-ahe-google
Copy link
Contributor

Stephen, why do you think this a dart2js bug?


Added NeedsInfo label.

@gramster
Copy link

unittest uses the stacks package, which uses the path package, which imports dart:mirrors.


cc @nex3.
cc @munificent.

@nex3
Copy link
Member

nex3 commented Aug 28, 2013

Why is it a problem that importing unittest disables tree-shaking? No one should be deploying code that imports unittest.

For reference, path is using dart:mirrors as a workaround for issue #6943.

@dgrove
Copy link
Contributor

dgrove commented Aug 28, 2013

path should probably be using the MirrorsUsed annotation to control this.

@gramster
Copy link

It is.

@gramster
Copy link

Nathan, I believe Stephen's concern is that the unittest tests could otherwise be useful for finding tree shaking bugs, but if we never tree shake, then we won't get that benefit.

@kasperl
Copy link

kasperl commented Sep 18, 2013

Added this to the M7 milestone.

@kasperl
Copy link

kasperl commented Sep 30, 2013

Removed Priority-Unassigned label.
Added Priority-Medium label.

@kasperl
Copy link

kasperl commented Oct 2, 2013

Removed this from the M7 milestone.
Added this to the M8 milestone.

@devoncarew
Copy link
Member

No one should be deploying code that imports unittest.

I have a use case for doing just that. I currently see a 10x increase in code size if I import unittest, which is less then ideal.

@peter-ahe-google
Copy link
Contributor

No one should be deploying code that imports unittest.

I have a use case for doing just that. I currently see a 10x increase in code
size if I import unittest, which is less then ideal.

If unittest is written under the assumption that it can force code to run in a mode that isn't appropriate for production, then you can't use unittest to test production code.

@dgrove
Copy link
Contributor

dgrove commented Oct 8, 2013

Uri.base is in, which is half of what's needed to get rid of this dependence on mirrors. I don't believe we have a way of knowing the platform (browser, mac, linux, windows) yet.

@kasperl
Copy link

kasperl commented Jun 4, 2014

Removed this from the M8 milestone.
Added this to the 1.6 milestone.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Removed this from the 1.6 milestone.
Added Oldschool-Milestone-1.6 label.

@rakudrama
Copy link
Member Author

I think this is no longer true.
Putting a throw at "isTreeShakingDisabled = true;" now fails only html/custom/mirrors_test


Added CannotReproduce label.

@rakudrama rakudrama added Type-Defect web-dart2js closed-cannot-reproduce Closed as we were unable to reproduce the reported issue labels Jul 29, 2014
@rakudrama rakudrama added this to the Oldschool-1.6 milestone Jul 29, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-cannot-reproduce Closed as we were unable to reproduce the reported issue web-dart2js
Projects
None yet
Development

No branches or pull requests

7 participants