reddit-moderator-toolbox icon indicating copy to clipboard operation
reddit-moderator-toolbox copied to clipboard

Restructure getThingInfo to use getters

Open eritbh opened this issue 6 years ago • 6 comments

Right now, getThingInfo uses a lot of DOM calls to get its information. We can speed this process up significantly by using getters around those calls instead of computing all the information when most of it won't be used after any one call anyway.

eritbh avatar Apr 14 '19 16:04 eritbh

Can we do this for after v5.1? Seems like we'd need to restructure all getinfo calls and frankly a bunch of them use all the info as tokens but can use at will for things like macros.

creesch avatar Apr 14 '19 16:04 creesch

This can be an incremental thing, it shouldn't change anything for consumers of the function. What I'm proposing is that we change stuff like

function getThingInfo ($thing, ...) {
	return {
        subreddit: $thing.attr('data-subreddit'),
        author: $thing.attr('data-author'),
        // etc
    };
}

with:

function getThingInfo ($thing, ...) {
    return {
		get subreddit () {
            return $thing.attr('data-subreddit');
		},
		get author () {
			return $thing.attr('data-author');
		},
	};
}

The resulting object will still let you do result.subreddit to get the sub name without a function call, but putting it in a getter means it's not computed until it's actually needed. This saves time because we don't end up computing properties that we never use.

eritbh avatar Apr 14 '19 16:04 eritbh

I see what you are getting at and am not opposed to it being implemented like that.

I am not entirely though sure if it makes a meaningful difference as at that point $thing is already an object and I am not entirely sure if any of the properties we fetch from it have a high performance impact.

creesch avatar Apr 14 '19 16:04 creesch

I don't have any evidence that it's a major slowdown either, but I suspect there may be things we do there that are more expensive than we realize. Whenever I get around to doing this I can run a benchmark and compare how much of a speed boost it actually gets us.

eritbh avatar Apr 14 '19 16:04 eritbh

but I suspect there may be things we do there that are more expensive than we realize.

No arguing there :D There is a lot of code in there made by people before they understood properly how DOM and performance worked.

creesch avatar Apr 14 '19 16:04 creesch

Looking back on my thought process here, I'm relatively sure that this won't have a major performance impact, and reimplementing the function to use this pattern will be touching a lot of stuff that doesn't really need to be messed with. The DOM interactions that really cause performance issues are manipulations that trigger a repaint or reflow; just getting data from the DOM shouldn't be limiting us anywhere. Closing this.

eritbh avatar Jan 30 '21 23:01 eritbh