These two bytes may not matter to you...

Nao

  • Dadman with a boy
  • Posts: 16,079
These two bytes may not matter to you...
« on August 5th, 2011, 12:31 PM »
(I decided I'd make this into a general purpose topic about my stupid micro-optimizations :P)

I had a look at addLoadEvent() in Wedge, and I can confirm that none of the calls use a string format at all. Heck -- it's actually used only 3 times in the entire codebase by now: to resize avatars on the fly, to resize images when clicking them, and to fix the code box sizes in FF, WebKit and IE. This last one I don't think even deserves being called through .load() but simply directly... Heck, I don't even know if the bug is still a valid one!

Thus, it even seems a bit odd to be using it when we could simply call $(window).load, although it's two bytes longer, but well, it also saves a function alias and declaring the function to begin with in script.js :P

Of course there's the possibility of modders calling addLoadEvent themselves, but... err... Since we're not compatible anymore... Maybe it'd be time for them to determine whether they want to use add_js() or DOMContentReady instead (i.e. anything that doesn't require images to finish loading...)

What do you think?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: These two bytes may not matter to you...
« Reply #1, on August 5th, 2011, 12:47 PM »
I've used addLoadEvent before but now that I think about it, it'd probably be OK with just the add_js stuff.

Put it this way, is there a compelling reason to keep it as it is? Does it perform a task that cannot be accomplished in another way? It sounds to me is though there is no task that it does that can't be done another way.
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: These two bytes may not matter to you...
« Reply #2, on August 5th, 2011, 03:31 PM »
addLoadEvent, as of now, is simply an alias to $(window).load(), which also carries the task of converting a string into a function (through new Function()). I was parsing the script.js file through JSHint (not JSLint) and wanted to fix a few of the notes (although on a performance note, this doesn't help at all so I won't use JSHint on all scripts in the end), and it did point out that Function() was compiled at execution time, and that it was bad, etc etc. So I went and looked for addLoadEvent calls, and it turned out I'd already converted all strings to functions, so this was only left in as emulation code. However, *Wedge is friggin' not compatible with SMF* to begin with. So I don't really see the point in having this function anymore... I'd rather encourage the use of other solutions ;)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

Nao

  • Dadman with a boy
  • Posts: 16,079

Rustybarnacle

  • Posts: 38

karlbenson

  • Posts: 44
Re: These two bytes may not matter to you...
« Reply #6, on August 5th, 2011, 08:26 PM »
Whilst I'm not familar with the functions you mention as alternatives IIRC the addLoadEvent was added because of pages with multiple triggered events.

The function was meant to enable modders to use so they stay clear from domcontentready or onload events.
We come in peace, shoot to kill, shoot to kill, shoot to kill; we come in peace, shoot to kill; Scotty, beam me up

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: These two bytes may not matter to you...
« Reply #7, on August 5th, 2011, 09:09 PM »
$(window).load(function) is really a jQuery way of adding a load event to the onload queue. So it effectively solves the multiple trigger issue ;)
Basically, anywhere I can use jQuery, I'll try to use it. (Except on performance critical functions. But I have yet to see a real one in SMF/Wedge :P)

Hey Karl, I was just working on your PM! I've got a large backlog of long PMs and posts to answer to... Can't work... ahhh >_<
Re: These two bytes may not matter to you...
« Reply #8, on August 6th, 2011, 08:17 PM »
On my to-do list is a rewrite of the forum version code...
The issue is that it's pretty messy. It's using $forum_version and WEDGE_VERSION (SMF_VERSION) to begin with. $forum_version is never redefined, so I don't see why we couldn't use WEDGE_VERSION everywhere. The only time it's defined out of the usual index files is in install.php where it's set when the recycle bin is enabled (???), and only used once, when it could just as well have used $current_wedge_version... Uh.

Now, $forum_version uses the format "forum version", rather than "version", while the const is just "version". Looking at the code, in most places $forum_version is actually parsed to remove the Wedge at the beginning... I really, really don't see the point in having Wedge in the name. What's it for? We're the only ones setting it, it's not like it's going away.

So... I'd suggest we (I) transform all $forum_version code to use just a version number. I've already done part of it. I'm also considering pushing it further and using WEDGE_VERSION instead, although the const could just as well be removed -- it's always defined, but never used once, except in upgrade.php which won't be used by Wedge anyway (uh.) Vars are slightly faster than consts, but it's not noticeable because it's not used in loops obviously.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: These two bytes may not matter to you...
« Reply #10, on August 6th, 2011, 10:54 PM »
Sorry, been limited on internet today.

I believe version emulation is the reason for it, that it has to be in a variable in order for it to work as currently implemented, plus the old copyright check.

I agree that just storing the version number (i.e. 0.1) in the constant is all that's actually needed, and we can insert 'Wedge' into the copyright string itself normally.

I'm still not convinced that constants are slower than variables, the only evidence I've seen for that is a claim to that effect without any real benchmarking to prove it, and even my own limited benchmarks don't seem to validate it being slower - if you're not throwing it around between scopes, and referencing it in only one major place per page load, the saving of not declaring both a variable and a constant should outweigh any performance issues from a constant, especially since that's the ideal context for using such a thing...

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: These two bytes may not matter to you...
« Reply #11, on August 7th, 2011, 01:01 AM »
- Did my own benchmark... Constant is faster for me, too, by about 15%.
- Haven't seen anywhere where $forum_version is redefined. As for package emulation, it first stores the version into $context['forum_version'] and then redefines it.

Going for the constant, then...! :)
Posted: August 6th, 2011, 11:33 PM

Just FYI -- code is cleaner now. Having no issues.
I noticed, however, that the 'detailed versions' admin page was broken. Is it also broken in the current dev?
This is due (among other things) to jQuery not supporting $('#randomidwithaperiod.likethis') as an ID name. Had to escape the period. Also, dozens of recently added files were not in the full list of files.

Honestly though -- I don't really see the point of retrieving a version Id for all files in the list. Is it to avoid forcing admins to upload all of their files again when just a few have changed? It's just annoying -- not updating the version number list, but updating the file list remotely!

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: These two bytes may not matter to you...
« Reply #12, on August 7th, 2011, 01:10 AM »
Quote
Honestly though -- I don't really see the point of retrieving a version Id for all files in the list. Is it to avoid forcing admins to upload all of their files again when just a few have changed? It's just annoying -- not updating the version number list, but updating the file list remotely!
It's more for when you have point releases like the 1.1.x line: each patch updates the version number of the files it's updated so you can tell what's what, since if a file doesn't change between 1.1.x and 1.1.y, they didn't update the file version number, so it makes sense then.

I'm inclined to think that maybe it could be ripped out.

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: These two bytes may not matter to you...
« Reply #13, on August 7th, 2011, 07:09 AM »
Here's the thing: I don't really see ourselves updating the file version manually on each upgrade ('did I update that file since the last update? I'll check against the last copy...'). It's tedious and will definitely be on our list of 'annoying things that don't encourage us to do early & frequent releases'. I'd rather we do a mass update on @version 0.1 to @version 0.2 etc...

So, basically, forcing people to reupload all files should they want to check all file versions in that area, doesn't seem like a big deal to me.
Or we could have Wedge auto-update its files itself, though some magical device... Although it wouldn't work on servers where the Apache owner can't overwrite files from the FTP owner ('nobody'? Or something else. Ayway. It's not like it happens everywhere.)

Why the heck am I awake and working at 7am on a Sunday...?!

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: These two bytes may not matter to you...
« Reply #14, on August 7th, 2011, 12:26 PM »
No, they don't have to upload all the files; the check is simply to validate whether what they have is the current version, since only the files that changed get new version numbers - if you're producing what amount to diff patches, it's trivial to add in the file versions.

To illustrate, I dug out the 1.1.12 install I had laying around. I'm not sure why I had it laying around but I did, and as it happens, it also has Aeva installed. It's not a patched version, it was installed as 1.1.12.

The results of the version check are as follows: (just keeping to sources to make the point)
Admin.php1.1.61.1.6
Aeva-Admin.php????
Aeva-Admin2.php????
Aeva-Admin3.php????
Aeva-Embed.php????
Aeva-Exif.php????
Aeva-Gallery.php????
Aeva-Gallery2.php????
Aeva-Media.php????
Aeva-ModCP.php????
Aeva-Sites-Custom-Example.php????
Aeva-Subs-Vital.php????
Aeva-Subs.php????
BoardIndex.php1.1.111.1.11
Calendar.php1.1.51.1.5
Display.php1.1.111.1.11
DumpDatabase.php1.1.121.1.12
Errors.php1.11.1
Help.php1.11.1
Karma.php1.1.51.1.5
Load.php1.1.111.1.11
LockTopic.php1.11.1
LogInOut.php1.1.61.1.6
ManageAttachments.php1.1.111.1.11
ManageBans.php1.1.101.1.10
ManageBoards.php1.1.21.1.2
ManageCalendar.php1.1.111.1.11
ManageErrors.php1.11.1
ManageMembergroups.php1.1.41.1.4
ManageMembers.php1.1.111.1.11
ManageNews.php1.1.51.1.13
ManagePermissions.php1.1.111.1.11
ManagePosts.php1.11.1
ManageRegistration.php1.1.101.1.10
ManageSearch.php1.1.121.1.12
ManageServer.php1.1.51.1.5
ManageSmileys.php1.1.121.1.12
Memberlist.php1.11.1
MessageIndex.php1.11.1
ModSettings.php1.11.1
Modlog.php1.1.111.1.11
MoveTopic.php1.11.1
News.php1.1.121.1.12
Notify.php1.11.1
PackageGet.php1.1.121.1.12
Packages.php1.1.111.1.11
PersonalMessage.php1.1.101.1.10
Poll.php1.1.111.1.11
Post.php1.1.111.1.11
Printpage.php1.11.1
Profile.php1.1.111.1.11
QueryString.php1.1.91.1.13
Recent.php1.1.51.1.5
Register.php1.1.101.1.10
Reminder.php1.1.61.1.6
RemoveTopic.php1.1.51.1.5
RepairBoards.php1.11.1
Reports.php1.11.1
SSI.php1.1.71.1.13
Search.php1.1.51.1.13
Security.php1.1.91.1.9
SendTopic.php1.11.1
SplitTopics.php1.1.111.1.11
Stats.php1.1.61.1.6
Subs-Aeva-Sites.php????
Subs-Auth.php1.1.111.1.11
Subs-Boards.php1.1.111.1.11
Subs-Charset.php1.11.1
Subs-Compat.php1.1.61.1.6
Subs-Graphics.php1.1.111.1.11
Subs-Members.php1.1.91.1.14
Subs-Package.php1.1.121.1.12
Subs-Post.php1.1.91.1.9
Subs-Sound.php1.1.61.1.6
Subs.php1.1.111.1.13
Themes.php1.1.111.1.11
ViewQuery.php1.11.1
Who.php1.11.1

This is what the system's designed for - each version only the changed files ever get updated and even in a quick release cycle this isn't necessarily a bad thing. And when you only change the headers on those files that updated, it means you can very easily see what's not been patched yet.

That said, the reality is that it isn't a lot of use because of file edits and if people do get into trouble they're often advised to simply upload fresh copies of everything. In our case the intention is that we'd do even more of that because we're aiming for less need of file editing. Perhaps it should just go after all.