WIP: Add typography performance tests#8918
Conversation
ksen0
left a comment
ksen0
left a comment
There was a problem hiding this comment.
Thanks for the work here! Comments below based on going through this PR with @limzykenneth .
The priority, especially if full completion is out of rant scope, is to complete the harness. If not possible to complete all the functions, these can be left open to community implementation in a new issue.
If possible, in addition to the minimal performance testing doc, it would be great to add instructions + example code for running manually (via CLI not the editor) a comparison between v1 and v2. As discussed, there is no equivalent quite for v1, but enabling comparison between v1 and v2 (even if manual / documentation primarily) would be ideall
| @@ -0,0 +1,65 @@ | |||
| # Typography Benchmarks | |||
There was a problem hiding this comment.
Thanks for including the WIP! In general the results do not need to be checked in. The documentation part can be put into a contributor doc on performance testing. (eg, here is the doc on unit testing, which is then included on the website)
|
|
||
| Implementation notes: | ||
|
|
||
| - All pre-existing benchmarks included the full p5 instance setup which adds ~50ms to each test. The `textToPoints() single word, isolated benchmark` test is included as a potential way to isolate performance testing away from setup overhead. |
There was a problem hiding this comment.
Same here: con be added to a new contributor doc on performance testing. That can be very short, so not necessary to add more than what you have here.
| } | ||
| } | ||
|
|
||
| const options = { iterations: 20, time: 500 }; |
There was a problem hiding this comment.
would be nice to use more iterations, of the runs 50ms is startup so when the numbers too close to 50ms then its less reliable/interpretable. Maybe 2x or more? To make it more noticeably larger than 50ms.
| options | ||
| ); | ||
|
|
||
| // TODO: This is an example of using setup() to isolate benchmarks away from p5 instance setup code |
There was a problem hiding this comment.
Possible to do everywhere? (Alternatively to upping iterations)
| options | ||
| ); | ||
|
|
||
| // TODO: textToContours() |
There was a problem hiding this comment.
If these are out of scope, rather than leaving them as inline todo's, you could make an open issue.
3 participants
Add initial performance benchmark tests and structure for typography tests.
PR Checklist
npm run lintpasses