Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reflex dom v0.4 keyed #240

Merged
merged 6 commits into from
Sep 26, 2017
Merged

Conversation

alexfmpe
Copy link
Contributor

@alexfmpe alexfmpe commented Aug 28, 2017

Working build (from another branch with extra commits to compile from source and skip other frameworks since I can't get master to run tests - see #239)

The benchmark has changed a bit since #214 and was meanwhile added to a branch of reflex-dom's repo.

npm install checks out the benchmark's source and build system
npm run build-prod is a no-op
npm run build-prod-force will recompile from source

The thing I'm most unhappy about is that the benchmark source/build files aren't actually added to this repo. Some alternatives to discuss:

  1. keep it as it is
  2. add the framework as a git submodule/git subtree (don't really have experience with these)
  3. duplicate the upstream benchmark into the PR

Number 3 is not DRY and more work when updating, but it's also the simplest.

I'm working with reflex-dom's author @ryantrinkle to add the benchmark to the framework's CI, so it's possible that the compiled files eventually also get extracted out and linked to (unless we pick number 2). Regardless, this will probably take a while.

FWIW, these are the results I got locally from running the tests with --count 1
image

@krausest
Copy link
Owner

I‘m on holidays this and next week - I‘ll take a look at it in two weeks.

@krausest
Copy link
Owner

I'd prefer npm install to also be a no-op and have a separate npm install-force. This would allow the install.js script to work unchanged. Is that ok for you? Can you update the pull request?

@alexfmpe
Copy link
Contributor Author

For some reason I put the dependency-fetch part in npm run build-prod-force, which makes npm install just a glorified git clone.
I'll move that part to npm (run) install-force tomorrow or so.
Also have to merge some small improvements from another branch (like the preload icon)

@alexfmpe
Copy link
Contributor Author

alexfmpe commented Sep 17, 2017

Applied requested changes. Should I squash this branch?

There are some improvements coming in the next weeks/months, but they're waiting for some CI work on reflex itself.

@krausest krausest merged commit 34ce2b0 into krausest:master Sep 26, 2017
@krausest
Copy link
Owner

Finally! It even found a bug in webdriverAccess.
The results for reflex dom can be found here.

@saurabhnanda
Copy link

Awesome 👍

@saurabhnanda
Copy link

And reflex is not the slowest :)

@ryantrinkle
Copy link

@saurabhnanda Thanks for helping push this through! It's clear we've got some weak spots, but I'm pretty pleased with some of the results, e.g. swap rows and remove row. We'll keep improving from here!

@alexanderkjeldaas
Copy link

I think this should include a round of closure

I'm using this:

CLOSURE_FLAGS    = --compilation_level=ADVANCED_OPTIMIZATIONS --jscomp_off=globalThis \
                   --jscomp_off=suspiciousCode --jscomp_off=uselessCode
 closure-compiler $(CLOSURE_FLAGS) \
           $(shell (stack path --local-install-root))/bin/krausest.jsexe/all.js > ../../../dist/all.js

@saurabhnanda
Copy link

@ryantrinkle thanks for supporting @alexfmpe while was working on this. And great to see that some of these benchmarks have been adopted into Reflex's build-chain as well.

😄

@ryantrinkle
Copy link

@saurabhnanda It was my pleasure; I'm glad we were able to get something in place that not only helped answer your questions, but will serve as a useful part of our infrastructure going forward. I'm hoping we can get performance testing integrated into our CI soon, which will really help when evaluating PRs.

@alexfmpe alexfmpe deleted the reflex-dom-v0.4-keyed branch October 28, 2017 21:23
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.

None yet

5 participants