Closed benmccann closed 4 years ago
i couldn't get this commit working (console errors).
but i locally compiled your parsing
branch as of https://github.com/chartjs/Chart.js/pull/6814 and got a 50% boost across the board vs Chart.js.html
:
Chart.js.html
740ms Scripting
195ms System
117mb peak
90mb retained
Chart.js3.html
383ms Scripting
95ms System
64mb peak
46mb retained
great strides!
@leeoniya my parsing
branch was committed to Chart.js master
. Does this PR work for you now? If not, what are the console errors you get?
@leeoniya was there anything else you wanted to do before merging this? Also, let me know if you have suggestions on the file name. It seemed a little funny to me, but I couldn't think of anything better
@leeoniya was there anything else you wanted to do before merging this?
no, this looks fine. i'll merge it when i have a bit more time for re-benching. i'm working on some uPlot API stuff that's higher in the queue. i see you may have more perf-related stuff coming up anyways: https://github.com/chartjs/Chart.js/pull/6822
@benmccann i should add that i would be hesitant to merge an implementation that comes with a bunch of caveats and disabled features, specifically made to move up in the bench tables. the fact that the fastline controller excludes datapoint rendering is not a good sign - what if i zoom in on a region that's sparse enough to show datapoints, am i SOL if i chose this controller originally?
is spanGaps handling also gutted?
i realize i'm judging the featureset from a biased point of view, but i don't want implementors carving out modes in their libs just so they can say "look, we can do a lineTo()
loop, too!".
curating benchmarks is hard.
Sure, that makes sense. Just to clarify, all the work I'm doing on improving performance is motivated primarily by my own use case. And this PR doesn't use the fastline
implementation since it sets type: 'line'
and not type: 'fastline'
I'm not entirely sure yet whether fastline
is something I'll end up pursuing or merging or what form it would take if I did. It was kind of a proof of concept I wrote on a lark. I found a couple interesting things from doing it such as a bug in the Chart.js code and also a change that would need to be made to the base controller to enable it. I think it's worth at least making some of these other supporting changes even if we don't pursue fastline
because it would enable users to create their own controllers based off of fastline
The main performance benefit of fastline
comes from not supporting animations. The reason I didn't include datapoint rendering or spanGaps was just because I didn't need it in my own project. I don't think it'd be hard to support if someone wanted it. I just didn't have a reason to do that work for my own project
the current non-minified version [1] works fine, but the minified version [2] gives me this:
[1] https://www.chartjs.org/dist/master/Chart.js [2] https://www.chartjs.org/dist/master/Chart.min.js
Hmm. I don't get that. I did "Ctrl + F5" to try to clear the cache and it still worked for me. Can you try that to see if maybe it was just a problem with an old version?
Can you try that to see if maybe it was just a problem with an old version?
what kind of dev do you take me for? 🤣
here's a fresh copy/pasta that fails for me: https://jsfiddle.net/80wabtvx/
Chrome Version 79.0.3945.79 (Official Build) (64-bit) Windows 10
it also breaks in Firefox.
what kind of dev do you take me for?
haha. I'm stumped though as to what else it could be
This is really weird because the jsfiddle works for me
@kurkle @etimberg could you guys give the fiddle link above a try? I'm not sure why it'd be failing for @leeoniya and working for me
fails on my phone too. maybe it's a cdn issue. can u attach the version u're getting? or content hash.
passes on my phone :-) here's the file my browser's loading (I had to append .txt
to the filename to get GitHub to allow me to upload it)
yes, it looks like a CDN issue. we're not getting the same versions.
i'm at home now (was in cafe before) and still have issues in both browsers. here's what i get served: Chart.min.js.txt. mine is coming from [2606:4700::6811:2035]:443
with the etag below. what's yours?
.
Wow. I never would have guessed that. Here's mine
so, any thoughts on how to move forward here? (it's still failing for me, btw).
Hmm. I wonder if you try on ipv4 if it will work?
@etimberg @kurkle do you guys know how https://chartjs.org/dist/master/Chart.min.js is hosted and deployed to Cloudflare? Is there anything you guys can think of to make sure the latest version is deployed and caching is disabled?
I wonder if you try on ipv4 if it will work?
it might, but that's irrelevant unless you can force all users of that link to disable ipv6.
edit: unless this can be disabled at the CDN level. i can try it sometime soon (i'm not sure if everywhere i tested it was over ipv6).
Yeah, I'm still trying to figure out what would cause that, so just looking for any useful clues
same issue on ipv4:
104.17.33.53:443 W/"5dd68267-2b1cf"
We didn't enable cloudflare, that must be something GitHub is doing for pages. When I test, I get
IP: 104.17.32.53:443
Etag: W/"5df03279-2bcc6"
cf-ray: 54636299aea4cab0-YYZ
x-served-by: cache-bwi5050-BWI
(this is super interesting. I am in YYZ so not sure why the cache is going to BWI)
It's in the chartjs.org DNS settings, so I think we did configure it. I'll email @nnnick and see if there's anything he can do to help
@leeoniya can you try again? I think this might be fixed now. Response I got from Nick:
I checked and the /dist files have a very high edge TTL, since a lot of people hotlink assets directly and usually the paths are canonical. Think tagged releases etc. Hadn’t considered the latest master branch, so I’ve updated to drop that to one hour for master branch.
yep, that did it.
Woohoo!
i'll re-bench this soon on Chrome 79.
meanwhile, Chrome just keeps getting slower and slower: https://github.com/krausest/js-framework-benchmark/issues/683
current numbers:
uPlot
42 console.timeEnd("chart")
71,6,3,72 script/rend/paint/sys (first render)
20.1MB -> 3.6MB heap (max -> retained)
153,517,129,259 script/rend/paint/sys (interact 10s)
Flot
320
192,5,5,306
39.6MB -> 17.1MB
---
dygraphs
170
239,4,5,172
113MB -> 64.1MB
2365,260,316,328
Chart.js-next
290
368,6,4,81
56.2MB -> 49.3MB
3705,36,93,4553
CanvasJS
321
398,5,4,103
47.9MB -> 37.4MB
2030,714,350,445
LightningChart
---
472,3,3,78
67.2MB -> 21.4MB
9725,25,47,98
jqChart
455
564,4,2,85
141MB -> 97.3MB
675,665,328,485
Highcharts
---
687,7,2,72
82.4MB -> 28.1MB
1150,581,194,276
Chart.js
680
726,6,4,175
113MB -> 90.8MB
5564,5,13,4067
ECharts
505
778,5,9,1074
182MB -> 124MB
2472,51,52,7333
ApexCharts
1265
2398,26,152,60
144MB -> 123MB
1815,205,8119,57
ZingChart
5858
6007,9,1,80
206MB -> 174MB
---
amCharts
6915
6953,37,12,102
441MB -> 441MB
4522,1237,2977,518
Cool! Is there anything else you wanted to do before merging this PR now that we have the new benchmark?
Thanks for all your help on this one! It was tricky to figure out that Cloudflare issue!
Is there anything else you wanted to do before merging this PR
i'm gonna pillage my markdown table builder from domvm [1] and maintain the benchmarks in a JSON file. i like the denser style of the non-html table (i need more columns) and formatting that manually is gonna get old quickly.
[1] https://github.com/domvm/domvm/blob/master/build.js#L234-L292
note to self: still need to link the benches from the new table & re-check lib sizes.
cool. i'm curious what's the difference between done
and js
in the next col?
done
== console.timeEnd("chart")
- this is just pure render time, excluding lib script parsing, JSON parsing & data prep.
js
is the the total amount of "scripting" recorded in the perf timeline summary from page load to done
(or somewhat after, assuming everything is settled).
the ordering in the chart is based subjectively on speed from "Rendering..." to the chart paint. Flot and dygraphs needed quite a few iterations to definitively place them - they're really close even though you can't tell that just from done
and js
. clearly, sys
and heap allocation provides the rest of the picture, but i haven't been able to get any consistent singular metric that would sort them in a way which reflects the actual experience.
Thanks for the detailed explanation! It's funny to me that "done" for Flot is greater than "js". I'd expect "js" to always be larger since it includes more. I probably would have chosen to order on the "js" column since data prep is normally done on the server side in my experience
I probably would have chosen to order on the "js" column since data prep is normally done on the server side in my experience
well, you can't exactly do data prep server-side for libs that need native Date
objects, so what then?
js
includes data prep & script parsing. since i just record a full perf timeline, i cannot - without a lot of effort - include script parsing (which is relevent, because less js should be rewarded) but also exclude the data prep that immediately follows it.
in theory, done
should be all that's needed (and for a lot of libs it is). but then there are cases where done
fires before the chart is painted. in other cases there libs load data urls (highcharts actually caches a png image of the plot, which takes time but i think happens after done
).
i was considering ordering everything by js + sys
, but that doesn't match the real experience. i think Chart.js-next does better than Flot in that sorting order, but if you actually open the page, Flot is very obviously faster. it'll never be perfect; it is what it is.
also, i updated the lib sizes and you guys put on some weight: 239 -> 247, but i don't recall 8k worth of features landing 🤷♂ .
Yeah, if anything I thought we deleted code. We added babble, so that could be part of it. I'll have to finish the buble PR. I'm surprised it's that big though because it was showing as only 174k in that PR for me: https://github.com/chartjs/Chart.js/pull/6836
i'm including all deps required to run the bench (besides what's inlined in the html) - so, luxeon
in your case and jQuery
for Flot.
It'd be cool to get this on the README if you're willing so we can see how we stack up