If you think that the main (non-themed) interface can benefit from these JS changes, please submit a Pull request!
1 Like
I can't wait for the Master Branch to have FTL.
I know, I know,,,,
Soon™
2 posts were merged into an existing topic: Dark Admin Theme
I'll do some more poking first and do a pull request.
Been reading over the implementation of AdminLTE, and no offence to anyone who has worked on it (as I know I can't talk, I'm no pro). But the implementation is VERY heavy and slow. It'd seem some have a habit of globally declaring things to the result of needing to loop through the global declarations just to find one element that is needed and strip it of anything unneeded. I understand it's a more global and including approach so it'd be easier to edit at a later date I suppose (except the files have been minified, but hey that's fine). But I don't think the cost of this implementation is worth it whatsoever. A prime example is what i stumpled upon with the graph colouring, i'll post examples...
This is the original code:
var colors = []; // Get colors from AdminLTE $.each($.AdminLTE.options.colors, function(key, value) { colors.push(value); }); var v = [], c = []; // Collect values and colors, immediately push individual labels $.each(data, function(key , value) { v.push(value); c.push(colors.shift()); queryTypeChart.data.labels.push(key.substr(6,key.length - 7)); }); // Build a single dataset with the data to be pushed var dd = {data: v, backgroundColor: c}; // and push it at once queryTypeChart.data.datasets.push(dd);
In pseudo code this is basically what that does:
create a variable (colors)
call an external variable declared in another file.
loop through several elements of that external variable.
push each element it finds to the variable we made (colors).
create two more variables.
loop for every value we need to push, whilst picking a colour from the 'colors' variable.
push the value and colour to the two new variables.
add a label name for the chart.
make another variable.
push the new variable to the chart.
Here's what I did. Yes it isn't as inclusive, or "universal". but you've seen the speed difference.
var colors = ["#37776A", "#f56954","#3c8dbc","#C874B1"];
var dd = {data:[],backgroundColor:[]}
$.each(data, function(key , value) { dd.data.push(value); dd.backgroundColor.push(colors.shift()); queryTypeChart.data.labels.push(key.substr(6,key.length - 7)); });
queryTypeChart.data.datasets.push(dd); queryTypeChart.chart.config.options.cutoutPercentage=30; queryTypeChart.update(); $("#query-types .overlay").remove();
Small change, only major difference is it excludes one of the heavy foreach loops which can take a little bit of time to run. Other than that it's also less resource hungry.
Point being, I can do a pull request for this, I don't know if the people that implemented it in the existing method would like it though. But if I were to do this I'd also wish to optimize other areas of the AdminLTE implementation.
1 Like
We'll definitely take a look at whatever you come up with. We just want to make sure that it doesn't introduce bugs or changes that might negatively affect later changes (introducing technical debt). The current code has many ways that it can be improved.
Depending on how many colors AdminLTE has, I wouldn't think that simply copying the array would be a big performance drain. How much time difference have you observed between the two versions?
It'd be quite tricky to observe the time it takes for that specific function to work as it's an API call too, so it varies too much to get a good base idea...
if it were me I'd do a small API rewrite that allows a json object of the specified types to be returned. Example:
api.php?type[]=getQueryTypes&type[]=overTimeData10mins&type[]= etc..
Reducing the amount of calls to the API and putting more pressure on the side of api.php would not only reduce the loading time by some. it'd also mean the graphs, charts etc will load almost simultaneously. Though I may be overthinking the need for efficiency.
With the next release, FTL will make loading times very quick, so that kind of optimization would not be necessary. With the current version, I think that change might even negatively affect the web interface, because with each new statistic to return, the API has to do even more work. With large logs it currently can get very close or even go past the timeout when the browser will close the connection. So if the commands are bunched up into one request, it most likely will never finish on large logs or even smaller ones because it has to do everything at once.
I've not read up on the FTL version yet. Good to know, I'll have a read now and I'll hold off from doing any changes I guess. timeouts are always an issue when using requests yes, even over a local stream. How big the object would be though? I will experiment locally to see, though I'm doubtful it'd take too much to pull the graph data needed for the dashboard.
It's generating the stats that takes the bulk of the time. With the current API, it has to re-parse the log file for every stat. With FTL, it does it in the background as the log file is written to, so that when the stats are requested it already has them generated.
1 Like
Ah nice, much more efficient. I'll hold off for sure then.
You can try it out if you like using:
sudo pihole checkout dev
Don't forget to do the usual browser cache flushing (also the JS cache which is most often separately) afterwards.
You can always revert back onto the master version using:
sudo pihole checkout master
1 Like
Just checked out the dev branch, Understand there's a long way to go but there's certainly some performance improvements in it. I'll make a dark theme for it shortly in another branch on my github so it'll be ready as the dev is ready.
The changes in the JS of the front end are small and mailny caused because we changed the style of the query types and forward destinations graphs into "over Time" graphs. The real speed benefit is that FTL is now responsible for all data that is delivered to the front end and it is very fast at doing this. I don't seen any issues with the JS that is there, currently, as the front page loads within much less than a second for me (running Pi-hole on original Raspberry Pi hardware).
That might be due to the fact that I only use quite powerful machines, running a fairly minimal Linux so there are no speed bottlenecks to be expected on the clients side of things (where the Javascript could only make a difference).
1 Like
Likewise I've a decent machine on mint. 0 chance of bottlenecking client side. Yet the difference did observable as many have. The changes in the Dev branch however will make these too small an issue to care about.
I still don't understand where the difference might come from - what takes long in the master version of Pi-hole is the computation of the statistics in real time from the logs on the Pi. Once that is finished, it is sent to the requesting client which displays it.
The difference in the development version (featuring FTL) is now that the statistics don't have to be computed, but are ready for transmission. They are delivered to the PHP API (which is the most noticeable bottleneck right now) und likewise delivered to the client for displaying from here.
So, you see that only the initial waiting period is killed by FTL. However, I cannot understand why any javascript changes can make things more fast, since the API will only deliver the data once ready and then everything together. That is only a small amount of data so transmission times should be in the low milliseconds range.
Nevertheless, I'd be very interested in a laoading time comparison with sufficient details, as e.g. obtainable using the Developer Tools on most modern browsers. That might show what makes the difference.
1 Like
Else than that, we are happy to include your theme (and any other nice theme from any user!) as an option on the Settings page. I guess it is as simple as submitting the modified CSS files under a different name as a PR. I'm afraid that it might be more complicated with the styling of the graphs, but we might find an elegant (modular) version for this as well.
If you are comfortable with this solution, we might be able to include your theme already in the soon (
) to be released Pi-hole v3.0 release (note that this is not a promise!).
2 Likes
just loaded it an do like it however and this may be a first time loading issue the forward destinations over time and query responses over time seems to be missing, also this does seem to take some of the speed of FTL away for me. Been testing FTL for a few months and have never seen it this slow, although slow is anything over a 1s load time nowadays
Do you mean the theme? The theme isn't made for FTL. I'll be making an FTL branch as it comes closer to release.
My theme is free to use, do as you wish. Though the charts point you raise is interesting. I'd say add an option under the UI section of the settings to alter the colours (simple colour picker) would be fairly easy to implement.