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

Updating npm packages and benchmarking #4201

Closed
3 of 15 tasks
limzykenneth opened this issue Dec 20, 2019 · 4 comments · Fixed by #4258
Closed
3 of 15 tasks

Updating npm packages and benchmarking #4201

limzykenneth opened this issue Dec 20, 2019 · 4 comments · Fixed by #4258

Comments

@limzykenneth
Copy link
Member

Nature of issue?

  • Found a bug
  • Existing feature enhancement
  • New feature request

Most appropriate sub-area of p5.js?

  • Color
  • Core/Environment/Rendering
  • Data
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Other (specify if possible)
    • Build system

Feature enhancement details:

As part of some exploration I'm doing I needed to benchmark existing p5.js code with another implementation but found that the benchmarking system is not working out of the box. I'm still working on getting it to work but it points to a larger work.

Specifically the build system's dependencies are getting out of date and getting bloated (which is briefly mentioned in #3409). So there's two proposals here:

  1. Refactor the build system including test and benchmarking tools, removing unnecessary dependencies. The aim is to preserve all front facing interface while streamlining everything else as well as updating outdated dependencies.

  2. Consider moving benchmarking to its own repo. Benchmarking probably hasn't work for awhile (not for me anyways) and looking critically at benchmarking it serves two potential purposes:
    a. To compare the speed to one implementation against another in the same environment (same browser, same machine, same test).
    b. To compare the speed of one browser with another for a specific implementation (Chrome vs Firefox, Chrome 78 vs Chrome 79, etc).

Benchmarking isn't useful for comparing performance of the implementation on one computer vs another computer as there will be too much variables that affect performance other that the implementation itself. Moving benchmarking to another repo we reduce the bloat in this main repo and on the new repo we can run on a consistent CI environment the above two use cases and be able to track the performance of p5.js code in a more consistent and complete way. While a user can also run the benchmarks locally while developing, if they are specifically working on optimisation for speed.

I'm going to work on the two proposals above but if there's any suggestions or comments about any of these, do post them below. I'll check them out. Thanks in advance!

@lmccart
Copy link
Member

lmccart commented Dec 20, 2019

@limzykenneth I think both proposals make total sense to me. My one suggestion is to consider, as you're working on #1, whether there's any documentation/inline comments that can be helpful. As we transition to the rotating leadership, I'm hoping to make what's there as clear as possible. Not trying to add work here. Just if things come up, even if you don't have time to fully document them, it would be helpful to note anything you can. I think overall the dependencies are pretty self explanatory, so this is more of a general note.

@limzykenneth
Copy link
Member Author

Yeah that sounds good, I'll add inline comments where it made sense.

@limzykenneth
Copy link
Member Author

I've rewrote benchmarking in a new repo here. It currently runs on Travis CI with Ubuntu 16.04 on headless Firfox and Chrome. I can't confirm the consistency yet, especially over longer periods of time.

There are still some features I would like to add but I'm having it on hold for now. If someone can check that out and if it's satisfactory, I'll refactor out benchmarking in this repo.

@lmccart
Copy link
Member

lmccart commented Jan 18, 2020

looks great! the detailed readme is very helpful. I think you can proceed with the refactor to remove from this repo. thanks @limzykenneth!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants