Skip to content

Conversation

@mwcampbell
Copy link
Contributor

Reasons to make this change:

  • Now Node can derive Default, which makes structure initialization more standard. Note: To allow this, Role also derives Default, with a default variant of Unknown. But I really didn't want to make NodeId derive Default, since that would mean allowing a zero ID, and multiple nodes with zero IDs would be an invalid tree.
  • In the persistent representation of the tree (in accesskit_consumer), the node IDs are already stored as keys in the map. So storing the IDs in the Node struct as well is redundant.
  • In a future PR, I plan to do a much larger refactor, introducing a trait called something like NodeProvider that defines getter methods for all of the node fields. The accesskit::Node struct, which I will probably rename, will implement this trait, but other implementations will be encouraged, for the sake of optimization. We will have to trust these implementations to be immutable snapshots, i.e. all tree changes must be done by passing new nodes to the adapter in a TreeUpdate. Forcing the node ID to be static, i.e. outside of the future trait, will eliminate one possible way that a broken provider could break the tree.

Unfortunately, this refactor requires lots of changes to test code, but I think it's worthwhile.

@mwcampbell
Copy link
Contributor Author

I'm abandoning this change because it increased the compiled code size, by about 12KB in a fully optimized release build of the Windows hello_world example. I think this is because I had to add the node ID to accesskit_consumer::Node, meaning the ID had to be copied into that struct in every inlined copy of node_by_id. I tried adding the ID to NodeState instead, which reverses one of the benefits listed in the first comment (because it requires a second copy of the node ID for every node in the tree), and even with that, this change increased the size of the binary by about 3KB. So I think it's not worth the trouble.

@mwcampbell mwcampbell closed this Oct 5, 2022
…ID in NodeState rather than copying it into consumer Node struct
@mwcampbell
Copy link
Contributor Author

I gave up on this too soon. I've decided that separating the node ID from the core Node struct, and the future trait with getter methods, is a better API design, even though it introduces new optimization issues. For now, I've worked around the worst of those issues by copying the node ID into NodeState, so it doesn't have to be copied into the consumer Node struct all over the place. We can do more optimization later. For now, it's important to get the API design right.

@mwcampbell mwcampbell reopened this Oct 5, 2022
Copy link
Member

@DataTriny DataTriny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think it's going in the right direction. I think this new TreeUpdate struct will be slightly less efficient when serialization is needed though.

@mwcampbell mwcampbell merged commit 0bb86dd into main Oct 7, 2022
@mwcampbell mwcampbell deleted the refactor-node-id branch October 7, 2022 14:01
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

Successfully merging this pull request may close these issues.

3 participants