Optimise vtag

[bakape]

Description

Optimized overall performance of VTag operations, mostly focusing on speeding up construction, reducing allocations and memory footprint.

Optimizations

• Enabled yew-macro to detect literals in most cases. These are set in VTag as static lifetime strings and thus notably reduce the need to allocate Strings (depends on HTML parametrization level). These include:
  • Attribute keys and values
  • HTML tag names
  • VText contents
• Reduced memory footprint of Vtag, by excluding impossible used field combinations using an enum.
• Removed key from VList. Arguably a very niche feature that still increased the memory footprint of VList and anything using it by 3 words.
• In-place non-dynamic VTag construction via yew-macro
• Inlined static class literal and href assignment into the attribute array construction to reduce possible reallocations
• Changed Attributes to be stored inside a Vec of key-value pairs. This has the benefit of not needing to construct HashMaps on VTag construction and that most scenarios will not encounter a change in attribute list length and key order.
• Added static if guards to dynamic tag name VTags to enable the compiler to more eagerly optimise out encased calls.

Breaking changes

• In-place VTag construction can create previously not present moved value usage conflicts.
• Removed key from VList.
• Removed kind attribute from VTag. It's use cases are now fully covered with the type attribute.

Checklist:

• I have run ./ci/run_stable_checks.sh
• I have reviewed my own code
• I have added tests

• I have opened a PR against https://github.com/yewstack/js-framework-benchmark: https://github.com/yewstack/js-framework-benchmark/pull/7/checks

[siku2]

Before I take a closer look at this, there are a few things I've noticed:

• There are a number of cases where the error span has regressed to Span::call_site (points at the entire macro invocation instead of pointing at a specific location)
• Using #v.to_string() emits less useful error messages than ::std::string::ToString::to_string(#v). This is true for all cases where you need to call an associated method.
• I don't think removing key from VList is acceptable. Admittedly, keys aren't really being used yet but they should bring major performance gains when used for big lists that change frequently.

As a whole this looks very promising but this PR does a little too much in my opinion.
Could you limit the scope of this PR to just the String optimizations for now?

[bakape]

There are a number of cases where the error span has regressed to Span::call_site (points at the entire macro invocation instead of pointing at a specific location)

I'll look into that.

Using #v.to_string() emits less useful error messages than ::std::string::ToString::to_string(#v). This is true for all cases where you need to call an associated method.

Noted.

I don't think removing key from VList is acceptable. Admittedly, keys aren't really being used yet but they should bring major performance gains when used for big lists that change frequently.

I might be misunderstanding the point of keys on VList then. Obvious what they are for on individual nodes but not quite for lists.

Could you limit the scope of this PR to just the String optimizations for now?

This PR is about reducing memory usage for VTag in general. I could separate static string usage, VTagInner and in-place construction into separate PRs, if needed. But I hardly see that as less work than reviewing one PR.

[siku2]

I might be misunderstanding the point of keys on VList then. Obvious what they are for on individual nodes but not quite for lists.

The thing is that fragments (lists) can also be members of a list like so:

html! {
  <key="my-list">
    <div />
    <key="item in my list">
      <span />
    </>
  <>
}

This PR is about reducing memory usage for VTag in general. I could separate static string usage, VTagInner and in-place construction into separate PRs, if needed. But I hardly see that as less work than reviewing one PR.

If you feel like splitting these up would be an unnecessary endeavour that's totally fine by me. Again, I haven't taken a closer look yet so I will take your word for it.

[teymour-aldridge]

Just a few preliminary thoughts I have. The most important one is about using a Cow.

[jstarry]

@bakape these optimizations look very promising!

This PR is about reducing memory usage for VTag in general. I could separate static string usage, VTagInner and in-place construction into separate PRs, if needed. But I hardly see that as less work than reviewing one PR.

I also prefer separate PRs. It's not about "less work" as it is about reducing cognitive overhead. Also, some of your changes are simple and can get merged quickly, while others are more complex or controversial and require more thought. I personally can always find time for a 100 line PR, but rarely get time to fully review a 1000 line PR.

It's definitely more work for you to split things up, but the alternative is this huge PR moves much more slowly.

[bakape]

@jstarry Okay, I will then do as suggested and split this PR, starting with the static string optimisations.

[jstarry]

Just submitted the last round of feedback, once these comments are addressed we can merge. Thank you!

[bakape]

@jstarry Done except for the attribute stderr regression.

[jstarry]

So awesome, thank you so much for this!

Pull request has been modified.

[jstarry]

I wasn't sure if these changes would impact the JS benchmarks but looks like they improved performance by about 5%!

https://www.dropbox.com/s/k6uzlh20gyd9gsv/Screen%20Shot%202020-08-22%20at%207.24.14%20PM.png?dl=0

[jstarry]

@bakape I just had another idea. What if we took static strings a step further? Right now we are using static strings for tag names but we have one static string per usage. It would be nice to reuse static strings.

Example

  <div>
    <span />
    <span />
    <span />
    <div>
      <span />
    </div>
  </div>

This example currently uses 2 static "div" and 4 static "span". Yew could export static strings for all common elements and then the macro could substitute those so that we only need one static "div" and one static "span". Thoughts?

[bakape]

@jstarry I have something similar to that planned as a VTagInner variant - so it depends on PRing and merging VTagInner first. The gist is you can have the procedural macro detect vtag trees with only literals at compile time and serialize those as HTML strings.

So

  <div>
    <span />
    <span />
    <span />
    <div>
      <span />
    </div>
  </div>

becomes

VTagInner::Static("<div><span></span><span></span><span></span><div><span></span></div></div>")

Diffing those is much much faster, because it's just string comparison, and they are applied to the DOM via a single set_outer_html() call.

I also have a global event listener multiplexer in the works.

[jstarry]

@bakape cool! I think we could still have the proc macro help with tags with dynamic attributes as well by reusing string literals.

I also have a global event listener multiplexer in the works.

Woot, this is going to be the killer feature for performance I think.

[bakape]

@jstarry Oh, I misunderstood what you meant in

This example currently uses 2 static "div" and 4 static "span". Yew could export static strings for all common elements and then the macro could substitute those so that we only need one static "div" and one static "span". Thoughts?

Would be very suprised, if the compiler actually creates 2 identical static strings. Let me check.

[bakape]

@jstarry It does not https://godbolt.org/z/1GbxPh

[jstarry]

Ah great, never mind then. I'll be sure to use godbolt more often, such a cool tool :)