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

branch: built-names #28

Open
jonathanolson opened this issue Aug 7, 2017 · 15 comments
Open

branch: built-names #28

jonathanolson opened this issue Aug 7, 2017 · 15 comments
Assignees

Comments

@jonathanolson
Copy link
Contributor

It looks like it's possible to use our Namespace.register function to "fix" the constructor names in mangled sims so that debug traces of built sims are more readable (e.g. "Dialog", "RoundPushButton" instead of "e").

There's a chance it could affect performance (if it prevents some optimizations) or that it might mess things up if function names are checked for other purposes, so pushing to a branch.

Tagged for developer meeting to decide if this is worth checking performance on all production platforms, and whether it would have the potential to break anything.

@jonathanolson
Copy link
Contributor Author

For the record, this would be valuable enough to me to recommend testing and merging to master.

@samreid
Copy link
Member

samreid commented Aug 9, 2017

It sounds promising to me, would be good to check for potential performance issues on iPad2.

@samreid
Copy link
Member

samreid commented Aug 9, 2017

I can see that the solution you proposed is straightforward, but I'm wondering if a better way to approach this would be to tinker with the mangle settings to keep constructor names.

@jonathanolson
Copy link
Contributor Author

but I'm wondering if a better way to approach this would be to tinker with the mangle settings to keep constructor names.

That's possible, but I'm curious how much that would increase the size of things (it would also expand any usage in the same file).

@jbphet
Copy link
Contributor

jbphet commented Aug 10, 2017

Discussed briefly in the 8/10/2017 dev meeting. The need for this hasn't come up for any of the devs who were present since bugs that only occur in the built version are rare, and the sim can be rebuilt without name mangling in that case. So, if this is inexpensive, it may still be worthwhile, but we'd like to understand the motivation better.

@samreid
Copy link
Member

samreid commented Aug 10, 2017

Maybe we should also look into using source maps, as they could provide better stack traces? This may become more important when PhET-iO has broader adoption and we are getting errors from unknown customizations (which are difficult to reproduce without the wrapper).

Or constructor.name as identified in this issue would be complementary to source maps?

@jbphet
Copy link
Contributor

jbphet commented Aug 10, 2017

Leaving tagged for dev meeting to discuss with @jonathanolson when he is around.

@jonathanolson
Copy link
Contributor Author

The need for this hasn't come up for any of the devs who were present since bugs that only occur in the built version are rare, and the sim can be rebuilt without name mangling in that case.

But bugs that happen only on specific platforms are somewhat common, and it's more difficult for anyone to just grab a matching device/browser, recompile with --mangle=false and run.

We've also had cases in a few sims where the original source SHAs were lost (so there would be no way I could rebuild the particular version run from source without mangling). When we can reproduce, it's still a pain that I can't really use stack traces (or the inspector / debugHTML) for production sims because the names are mangled.

Or constructor.name as identified in this issue would be complementary to source maps?

It could be completely complementary. This small line-change adds a lot of functionality above and makes stack traces / inspection way more helpful.

@samreid
Copy link
Member

samreid commented Aug 11, 2017

It sounds great to me, we should sanity check it for performance/memory issues and go for it.

@pixelzoom pixelzoom changed the title [branch] built-names branch: built-names Aug 14, 2017
@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 14, 2017

I changed the title of this issue to match the convention that we use for branch issues. That is "branch: {{BRANCH_NAME}}". Following the convention makes it easier to search for branch issues.

@jonathanolson
Copy link
Contributor Author

I changed the title of this issue to match the convention that we use for branch issues. That is "branch: {{BRANCH_NAME}}". Following the convention makes it easier to search for branch issues.

Doh, I must have unintentionally changed to thinking that was the proper convention. I changed titles on 6 other open issues. Is it worth changing closed branch issues to that format?

@pixelzoom
Copy link
Contributor

Is it worth changing closed branch issues to that format?

Up to you. My feeling is no.

@pixelzoom
Copy link
Contributor

8/17/17 dev notes:
• sounds safe
• unlikely to leak any IP related to phet-io
• check iPad2 performance before integrating into master (check startup time + animation time)

@jonathanolson
Copy link
Contributor Author

Won't launch on iPad, so I'm leaning towards deleting this branch, and building without mangling when I need names. Any objections?

@samreid
Copy link
Member

samreid commented Aug 17, 2017

It seems like it is worth at least 30 minutes to try to understand why this could fail so badly on iPad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants