Skip to content
This repository has been archived by the owner on Jul 27, 2020. It is now read-only.

Set property directly, rather than using defineProperty #136

Merged
merged 3 commits into from
Jun 2, 2015

Conversation

ironsidevsquincy
Copy link

Causing a big performance hit, e.g. every time this.body was being called, the function was run.

Starting with just this, but all the properties in here should set this directly

Using Engels as a test,

Before

$ siege -c 12 -t 20s http://localhost:3002/uk
...
Transactions:                 40 hits
Availability:             100.00 %
Elapsed time:              19.40 secs
Data transferred:          12.02 MB
Response time:              4.67 secs
Transaction rate:           2.06 trans/sec
Throughput:             0.62 MB/sec
Concurrency:                9.62
Successful transactions:          40
Failed transactions:               0
Longest transaction:            6.59
Shortest transaction:           1.73

After

$ siege -c 12 -t 20s http://localhost:3002/uk
...
Transactions:                 91 hits
Availability:             100.00 %
Elapsed time:              19.48 secs
Data transferred:          27.36 MB
Response time:              2.01 secs
Transaction rate:           4.67 trans/sec
Throughput:             1.40 MB/sec
Concurrency:                9.39
Successful transactions:          91
Failed transactions:               0
Longest transaction:            2.80
Shortest transaction:           1.21

@matthew-andrews
Copy link

wow, great work. i never liked defineProperty. too magic.

ironsidevsquincy added a commit that referenced this pull request Jun 2, 2015
Set property directly, rather than using defineProperty
@ironsidevsquincy ironsidevsquincy merged commit 4418d7b into master Jun 2, 2015
@wheresrhys
Copy link
Contributor

I think defineProperty is still useful for expensive actions that don't need to be carried out for most instances, but the vast majority of the properties here don't meet this criteria

@i-like-robots
Copy link

I use defineProperty a few times in the related tags API for creating objects with dynamic key names, worth testing for speed?

https://github.com/Financial-Times/next-ft-api-client/blob/master/src/related-tags.js#L30-40

@phamann
Copy link

phamann commented Jun 3, 2015

Nice!

@ironsidevsquincy
Copy link
Author

It's not defineProperty that's inherently slow, it's just every time a property is called it runs the function, only to return a constant value in our case.

Particular example is this.readingTime was being called, which in turn was calling this.wordCount, which in turn was calling this.body, which was running the intensive dom.removeNonArticleLinks(dom.fixRelativeLinks(dom.removeEmptyParagraphs(html))) every time, even though these values don't change. And when you've got this, which was calling readingTime up to 3 times, you get a major slowdown

Basically, only use defineProperty if you need to watch the setting/getting of a property, of the property changes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants