Skip to content

Conversation

@bakape
Copy link
Contributor

@bakape bakape commented Jul 8, 2021

Description

#1905 overwrote the optimizations merged in #1867. This PR remerges those optimizations and additionally:

  • Moves more VTag construction logic to compile-time via the html! macro
  • Reduces VTag memory footprint via enums
  • Reduces enum branching during VTag patching. This includes the various Option<T> values already present before this PR.

Benchmark results using https://github.com/bakape/js-framework-benchmark
Screenshot_20210708_233834

The small degradation of row swapping performance can be addressed by merging #1555 to improve VList patching vectorization.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@bakape bakape changed the title Fix/remerge vtag construction opt Optimize VTag construction, memory footprint and patching Jul 8, 2021
@bakape
Copy link
Contributor Author

bakape commented Jul 9, 2021

Is support for Rust 1.45 still required? It's almost a year old release by this point.

@teymour-aldridge
Copy link
Contributor

I thought we'd bumped the MSRV to 1.51.0 991abab

@bakape
Copy link
Contributor Author

bakape commented Jul 9, 2021

@teymour-aldridge Seems this was missed in that PR: https://github.com/yewstack/yew/blob/master/.github/workflows/pull-request.yml#L131.
I'll PR a fix.

@ranile
Copy link
Member

ranile commented Jul 9, 2021

Nah, we just bumped the version for tests in CI. MSRV is still the same

@bakape
Copy link
Contributor Author

bakape commented Jul 9, 2021

Hmm, should I amend my code with some unsafe then? The alternatives would be conditional compilation on rust version or some performance penalty by adding std::mem::take.

@teymour-aldridge
Copy link
Contributor

teymour-aldridge commented Jul 9, 2021 via email

@teymour-aldridge
Copy link
Contributor

teymour-aldridge commented Jul 9, 2021 via email

@cecton
Copy link
Contributor

cecton commented Jul 10, 2021

I'm totally fine bumping the MSRV version but I think it's best to have an older maintainer than me on this subject.

@jstarry
Copy link
Member

jstarry commented Jul 10, 2021

Bump MSRV as needed, it's just added as an FYI to any consumers of the library 😄

@github-actions
Copy link

github-actions bot commented Jul 11, 2021

Visit the preview URL for this PR (updated for commit c4f62ff):

https://yew-rs--pr1947-fix-remerge-vtag-con-bwhafw1o.web.app

(expires Sun, 25 Jul 2021 16:20:48 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

cecton
cecton previously approved these changes Jul 12, 2021
Copy link
Contributor

@cecton cecton left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 One more reviewer would be nice

@mergify mergify bot dismissed cecton’s stale review July 18, 2021 12:07

Pull request has been modified.

@bakape
Copy link
Contributor Author

bakape commented Jul 18, 2021

@siku2 Everything addressed.

@siku2 siku2 merged commit d89f1cc into yewstack:master Jul 18, 2021
@voidpumpkin voidpumpkin added the A-yew Area: The main yew crate label Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-yew Area: The main yew crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants