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

Should all scenery Nodes always allocate all a11y keys and values? #1196

Open
samreid opened this issue Apr 12, 2021 · 9 comments
Open

Should all scenery Nodes always allocate all a11y keys and values? #1196

samreid opened this issue Apr 12, 2021 · 9 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Apr 12, 2021

In #1159 I saw that all Nodes include the following:

this._tagName = null;
this._containerTagName = null;
this._labelTagName = null;
this._descriptionTagName = null;
this._inputType = null;
this._inputValue = null;
this._pdomChecked = false;
this._appendLabel = false;
this._appendDescription = false;
this._pdomAttributes = [];
this._labelContent = null;
this._innerContent = null;
this._descriptionContent = null;
this._pdomNamespace = null;
this._ariaLabel = null;
this._ariaRole = null;
this._containerAriaRole = null;
this._ariaValueText = null;
this._ariaLabelledbyAssociations = [];
this._nodesThatAreAriaLabelledbyThisNode = [];
this._ariaDescribedbyAssociations = [];
this._nodesThatAreAriaDescribedbyThisNode = [];
this._activeDescendantAssociations = [];
this._nodesThatAreActiveDescendantToThisNode = [];
this._focusableOverride = null;
this._focusHighlight = null;
this._focusHighlightLayerable = false;
this._groupFocusHighlight = false;
this._pdomVisible = true;
this._pdomOrder = null;
this._pdomParent = null;
this._pdomTransformSourceNode = null;
this._pdomDisplaysInfo = new PDOMDisplaysInfo( this );
this._pdomInstances = [];
this._positionInPDOM = false;
this.excludeLabelSiblingFromInput = false;
this._accessibleName = null;
this._accessibleNameBehavior = ParallelDOM.BASIC_ACCESSIBLE_NAME_BEHAVIOR;
this._helpText = null;
this._helpTextBehavior = ParallelDOM.HELP_TEXT_AFTER_CONTENT;
this._pdomHeading = null;
this._headingLevel = null;
this._pdomHeadingBehavior = DEFAULT_PDOM_HEADING_BEHAVIOR;
this.focusHighlightChangedEmitter = new TinyEmitter();
this.pdomDisplaysEmitter = new TinyEmitter();

For other attributes added to Node.js, we are sometimes careful to make sure they lightweight, and we make measurements before and after to see the impact on heap or startup time. For example, see #1096. Has such an analysis been done for this contibution?

In the case of PhetioObject, we employed a pattern where PhetioObject instances can remain lightweight until they are optionally triggered to become heavyweight. It looks like ParallelDOM eagerly allocates everything, and that all instances are always heavyweight. Should we use a similar pattern where Node has (by composition) a placeholder for PDOM sub-object (defaulted to null), which is only triggered to be allocated once you start using PDOM features for that Node?

@jonathanolson
Copy link
Contributor

I think this can honestly generalize to a lot of things where (a) they aren't used often, and (b) the access doesn't need to be super-high-performance.

The Node hints (which can probably spare an extra redirection for getters) could also potentially speed things up.

@jonathanolson jonathanolson self-assigned this Apr 12, 2021
@jonathanolson
Copy link
Contributor

I'd like to be part of this, as I think I would definitely generalize it to other parts of Node for potentially significant savings.

@jessegreenberg
Copy link
Contributor

To get a sense of how much ParallelDOM.js adds to Node.js I did a test comparing the heap snapshot before/after creating 10,000 Nodes on master against a local copy where ParallelDOM is not mixed into Node.

Without ParallelDOM.js:
image

With ParallelDOM.js (master)
with

So over 10,000 Nodes that is a 9.24% increase in size for ParallelDOM. I also noticed it took substantially longer to construct 10,000 Nodes on master vs. with ParallelDOM removed (~1 second without, ~45 seconds with).

@jonathanolson
Copy link
Contributor

What about strategies for lazily handling these? Would we group things into lazily-created objects? Would we prefer to have them be defined ON the node, BUT have a flag of whether they have been set? (Will all getters need to return default values if it's not created?)

@zepumph
Copy link
Member

zepumph commented Apr 13, 2021

In PhetioObject we were able to conditionally define much of the API based on brand. That was nice, but we likely don't have such an easily checkable condition for SCENERY/Node.

@jonathanolson
Copy link
Contributor

In PhetioObject we were able to conditionally define much of the API based on brand.

It still seems like things are taking up a lot of memory on the object, and for performance costs. Is that done at all in phet-io?

@zepumph
Copy link
Member

zepumph commented Apr 13, 2021

Nope! My first thought for this issue was to try to make it more general. If we have a process for conditionally defining things that we could get away with lazily creating, and could easily create a pattern to use that in multiple types, that would be excellent! I don't really see a way to though.

@jonathanolson
Copy link
Contributor

I think I have a potential way for doing this that I'd like to prototype.

@jonathanolson
Copy link
Contributor

I have a somewhat-crazy idea half-prototyped, that:

  1. Would lazily store only non-default values for fields
  2. Supports lazily-created Properties for any field

I'm curious how desirable (2) would be. I can imagine it might be useful, but also adds some complexity (since the before/after/equality needs to be provided). I've also coincidentally solved the type-checking and value-coercing problems @pixelzoom and I have run into (so that type checks are run directly on Property sets, and if it coerces its value, that's what would be stored and have equality checked against).

A core part of this is... something like TinyProperty that doesn't store its own value, but instead looks it up in the lazy storage (and itself can be created lazily, so you'd only have THAT Property in cases where you actually want to deal with the Property).

Maybe tomorrow I could run this by both of you, to see if I should flesh this out? It should reduce memory impact significantly, will improve object construction performance, but getting/setting will be slower. I'd probably want to continue the prototype enough to measure performance benefits.

jonathanolson added a commit to phetsims/tandem that referenced this issue Apr 22, 2021
jonathanolson added a commit to phetsims/axon that referenced this issue Apr 22, 2021
jonathanolson added a commit that referenced this issue Apr 22, 2021
@zepumph zepumph removed their assignment Mar 3, 2023
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

4 participants