dodrio icon indicating copy to clipboard operation
dodrio copied to clipboard

Investigate adding a change list ops for common attributes and listeners

Open fitzgen opened this issue 6 years ago • 2 comments

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 avatar Mar 02 '19 01:03 fitzgen

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.

theduke avatar Apr 07 '19 21:04 theduke

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: https://github.com/fitzgen/dodrio/pull/35

fitzgen avatar Apr 08 '19 17:04 fitzgen