Skip to content
This repository has been archived by the owner on Mar 2, 2021. It is now read-only.

Investigate adding a change list ops for common attributes and listeners #27

Open
fitzgen opened this issue Mar 2, 2019 · 2 comments
Open
Labels
performance Related to performance and profiling

Comments

@fitzgen
Copy link
Owner

fitzgen commented Mar 2, 2019

The "class" and "id" attributes are used super frequently. We could have a SetKnownAttribute op that has an immediate that is an enum value for various common attributes instead of the string for the attribute. This would save time decoding strings in the change list implementation, which is pretty hot.

We could do a similar thing for known events, like "click" etc.

The trade off is that the more "known" events and attributes we support, the more we have to check whether an attribute is known or not when diffing and emitting the change list. But some initial profiling shows that we only spend about 5% of time in diffing, and much than that in applying diffs. Even just string decoding within diff application we seem to spend about 10% of time in!

@fitzgen fitzgen added the performance Related to performance and profiling label Mar 2, 2019
@theduke
Copy link

theduke commented Apr 7, 2019

Have you considered typed ElementNode and Attribute versions instead?

as in:

enum TagName<'a> {
  Div,
  P,
  ...,
  Other(&'a str),
}

enum AttributeName<'a> {
  Class,
  OnClick,
  ...
  Other('a str),
}

struct ElementNode<'a> {
  tag_name: TagName<'a>,
  ...
}

The Other variants would be a very rare edge case for custom attributes etc.

That would avoid string comparisons both during diffing and while applying the changelist.

@fitzgen
Copy link
Owner Author

fitzgen commented Apr 8, 2019

Yes I have. The primary downside there is the size increase this would have on Node.

I think we've probably reaped most of the wins here since this issue was opened by introducing a generic strings cache that is synced between the change list compiler in wasm and the change list interpreter in js: #35

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
performance Related to performance and profiling
Projects
None yet
Development

No branches or pull requests

2 participants