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

Performance audit for Node.phetioVisibleProperty definition in constructor #1055

Closed
zepumph opened this issue Jun 3, 2020 · 1 comment
Closed
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Jun 3, 2020

From #1046 and #490 (comment):

@jonathanolson said:

[@zepmuph said] In Node.js we define this.phetioVisibleProperty. We weren't sure whether to null it out in the constructor (adding more keys to every single Node) or to lazily add it to only the Nodes that are PhET-iO instrumented. Keep in mind this will be done for opacity and pickability as well. We weren't sure whether to minimize the number of Node keys or hidden classes. It seems clearer to put them in the constructor, but we weren't sure how essential it is to minimize the number of Node attributes.

That is a really good question. Because we are inheriting from Node all the time, it complicates the hidden class question (so intuitively I think the minimization of Node keys might be better for performance/memory, but I haven't investigated that). Sounds like something good to test, to see what effect it would have.

I think this should be in a separate issue. @jonathanolson please let me know if I can help with this. Currently it is not in the Node constructor, even set to null.

@zepumph
Copy link
Member Author

zepumph commented Oct 7, 2020

Over in #1087 we decided to initialize it to null in the constructor. It is now called ownedPhetioVisibleProperty. Closing

@zepumph zepumph closed this as completed Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants