Skip to content
This repository has been archived by the owner on Dec 19, 2017. It is now read-only.

DO NOT MERGE: add noSuchMethod proxying to the js element #76

Closed

Conversation

jakemac53
Copy link
Contributor

I just wanted to get some feedback from you guys on doing something like this. I understand that it will have a potentially significant impact on the performance, but it would enable generic support for methods which is a pretty big win and would alleviate a lot of the pain that people are seeing since functions seem to be rarely annotated in on the JS side and thus don't get properly wrapped by our code generator.

I am mostly curious about whether we have any actual benchmarks on how this affects the code. Is it worth my time to see how this affects the dart2js output?

@sigmundch
Copy link
Contributor

Unfortunately this would be a big cost in the dart2js output. I'm happy to talk more about it tomorrow, but there are basically 2 issues here:

  • noSuchMethod has a large size overhead on applications. On a small TodoMVC app I was seeing about 15% of size overhead or more (I can't recall the exact numbers right now).
    • dart:mirrors also add a lot of overhead. You could annotate it to indicate that you only need to translate method names to symbols, but that alone can be big. That's why our polymer build step does a lot of work to get rid of every API call to the mirror system and replace it with uses of smoke.

There are parts of the smoke/builder implementation that would have been simpler if we used noSuchMethod, but we decided not to use them for the same reasons.

@zoechi
Copy link

zoechi commented Jul 23, 2014

What about just creating PRs for the JS core- and paper-elements for the missing annotations?
I didn't look at the code generation code yet. Is it documented somewhere what annotations are necessary?

@jakemac53
Copy link
Contributor Author

Currently the strategy is to do exactly that, just make PRs to add annotations to the JS side of things. However this is a slow and reactive process, and sometimes there may be situations where the JS polymer guys don't want to document a method publicly for some reason, see googlearchive/core-drawer-panel#10 for example. While this did get merged it was ambiguous whether or not they might push back on it in the future if a method is not deemed ready for publishing (even though users might be already relying on it).

That being said, we could make a push to go through all of their elements and submit PRs to add annotations. Once everything is in a good state then we should be able to keep up with it during each new roll of the elements, although it will likely add 1-2 days to that process to wait for the changes to get merged upstream each time.

In terms of what the supported annotations are, this is not officially documented anywhere that I know of, but you can see all the options in this switch statement https://github.com/dart-lang/core-elements/blob/master/lib/src/parser.dart#L109.

@jakemac53
Copy link
Contributor Author

Also, I checked quickly and the dart2js output size has increased 25% overall, and up to almost 50% on some of the bootstrap.js files after this change. I would say that is definitely unacceptable so I am closing this PR, although we can continue discussing the proper solution to this.

@jakemac53 jakemac53 closed this Jul 23, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants