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

interfaces_assign.php UI speedup as requested on forum bounty board #3868

Merged
merged 5 commits into from Nov 29, 2017

Conversation

Projects
None yet
3 participants
@loonylion
Copy link
Contributor

commented Nov 12, 2017

Improves significantly and hopefully fixes bug#6400

Refactored interfaces_assign.php to to benefit people with large numbers of VLANs, as requested on the forum at https://forum.pfsense.org/index.php?topic=137391.0. Also contains a minor speedup for interfaces_vlan.php. Modified functions are contained in interfaces_fast.inc. Profiling code is still present but commented out, as is replaced code.

loonylion added some commits Nov 12, 2017

refactored interfaces_assign.php to to benefit people with large numb…
…ers of VLANs, as requested on the forum at https://forum.pfsense.org/index.php?topic=137391.0. Also contains a minor speedup for interfaces_vlan.php. Modified functions are contained in interfaces_fast.inc. Profiling code is still present but commented out, as is replaced code.
Improves bug#6400 further reducing page load time from my previous co…
…mmit

Moved select box generation code out of interface display loop, meaning it runs once iterating over count(interfaces) rather than running count(interfaces) times iterating over count(interfaces) each time.

Also removed tabs and some newlines from <select><option> code as this significantly reduces page size and load time. <option> code now left justified (no indent) and on one line per option, for a saving of ~9KB per line and ~4.5MB overall with 500 VLANs
@loonylion

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2017

New commit also removes most commented code that was replaced in the first commit, some of which appeared to be slowing down page load significantly. Page load times are now much closer to generation times than previously. Tested up to 1001 VLANs, page load time of around 1 minute.

* Functions in this file may not require/use parameters when called, however
* in most cases they will accept parameters as per their original forms, for
* consistency.

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

Please use current (Apache) license text (see all other php files)

@sbeaver-netgate

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

Incorrect license text in new file

@loonylion

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

licence text changed as requested

require_once('interfaces.inc');
/*
* does_interface_exist_fast
Returns an array of interrfaces which exist on the system.

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

Spelling: interrfaces

* convert_real_interface_to_friendly_interface_name_fast($interface): convert fxp0 -> wan, etc.
Returns an array of interfaces and friendly names.
Doesn't actually use any parameters, they're just accepted for consistency with the function it replaces
*/

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

I don't think this consistency is required, and it rather obfuscates the code. Could these arguments be removed wherever used?

write_config();
/*$timeb = microtime(true);
$profile['post_write_config']=$timeb-$timea;*/

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

Let's just remove stuff that is no longer required.

/* this code scales much much better
0.0065770149230957 seconds
vs
0.49271988868713 seconds with 400 vlans*/

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

Comment not useful here

This is too much of an improvement to ignore. Tested with 500
VLANS, total page output is 17,197.33KB with 8456ms spent
waiting for page generation and transmission, compared to
21,697.89KB with a wait of 9734ms*/

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

This is a self-congratulatory comment. "Keep HTML options on one line to minimize size/load-time" would suffice.

/* replacing the port select menu generation loop that has count(interfaces) iterations
and is run count(interfaces) times with a pre-prepared select menu generated outside of
this loop has produced a significant improvement in page generation and load time */
//foreach ($portlist as $portname => $portinfo):

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

A more concise comment would be nice.

/* As with the gettext() calls, I've removed the interface_assign_description() calls and
replaced them with my own interface_assign_description_fast() function that's called once
outside of the loop. Also like the gettext() edits, this change keeps paying for itself.
Also, removing the indents and newlines saves potentially ~9KB of HTML with 500 unassigned VLANs */

This comment has been minimized.

Copy link
@sbeaver-netgate

sbeaver-netgate Nov 29, 2017

Contributor

Comment not helpful here. "HTML indenting not used to save tmie/space" would suffice.

interfaces_fast.inc: removed accidental rolling 'r' from comment
                     removed unused parameters from definition of convert_real_interface_to_friendly_interface_name_fast()

interfaces_assign.php: removed profiling code
                       removed unncessary comments
                       shortened some comment blocks
                       removed parameters from a function call that didn't use them
@loonylion

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

changes made, the long comments were intended to explain my reasoning behind some of the more significant changes or changes that appeared to possibly conflict with the coding style guide (in the case of the indent removals)

@sbeaver-netgate

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

Thanks. - Merged.
Those comments are better here than in the source I think :)

@netgate-git-updates netgate-git-updates merged commit d7dc67f into pfsense:master Nov 29, 2017

@sbeaver-netgate

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

Appears to be causing numerous issues: https://redmine.pfsense.org/issues/8223

@sbeaver-netgate

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

PR was reverted due to errors introduced.

netgate-git-updates pushed a commit that referenced this pull request Dec 18, 2017

Revert "Merge pull request #3868 from loonylion/master"
Caused issues reported in https://redmine.pfsense.org/issues/8223
This reverts commit 74c5525, reversing
changes made to 2acb402.
@loonylion

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

Both bugs should be fixed now. I reverted back to stock pfSense code for naming OPT interfaces since the improvement wasn't huge anyway, which should take care of bug #8222 and I corrected the broken reference causing bug #8223

@loonylion loonylion referenced this pull request Jan 3, 2018

Closed

fix bugs #3894

3 of 3 tasks complete

loonylion added a commit to loonylion/pfsense that referenced this pull request Jan 8, 2018

re-adding changes made to fix bug#6400, includes fixes for bug#8222 a…
…nd bug#8223 that were introduced with the initial commit of this code.

original pull request was pfsense#3868
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.