-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Simpify getting object type name #7159
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7159 +/- ##
==========================================
+ Coverage 66.43% 66.44% +<.01%
==========================================
Files 253 253
Lines 10658 10664 +6
Branches 4 4
==========================================
+ Hits 7081 7086 +5
- Misses 3576 3577 +1
Partials 1 1
Continue to review full report at Codecov.
|
function isA(typeName: string, value: any): boolean { | ||
return Object.prototype.toString.apply(value) === '[object ' + typeName + ']'; | ||
function getObjectType(value: any): string { | ||
return Object.prototype.toString.apply(value).slice(8, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any perf benchmarks showing that this slice is not slowing down the whole caching effort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slice(8, -1)
works 4x faster than concatanation '[object ' + typeName + ']'
.
Source of my test.
const map1 = new Map();
const map2 = new Map();
let start;
function isA(typeName, value) {
return Object.prototype.toString.apply(value) === '[object ' + typeName + ']';
}
function getObjectType(value) {
return Object.prototype.toString.apply(value).slice(8, -1);
}
console.log('isA');
console.log('=======================');
start = process.hrtime()[1];
const compare1 = isA('Map', map1);
console.log(process.hrtime()[1] - start, compare1);
console.log('');
console.log('getObjectType');
console.log('=======================');
start = process.hrtime()[1];
const compare2 = getObjectType(map1) === 'Map';
console.log(process.hrtime()[1] - start, compare2);
Result of my tests (nano seconds):
isA
=======================
208620 true
getObjectType
=======================
57268 true
Tested on Node v10.10.0, MacOs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is single execution without a warmup, hard to tell if it's representative. Can your run it on jsperf? Something like here https://twitter.com/bmeurer/status/867277137564360704?s=21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gain in jest's case is probably from doing the toString
once instead of up to 12 times.
I doubt slice
or not makes much of a difference. I don't think this is super hot code anyways - it's only used when generating mocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. full test.
Test compares original getType
(with many isA calls) and getType
from this PR with chaching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing changelog entry and we're good? :) |
I updated changelog. @thymikee |
Thanks! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
There is function for checking object type
But this function called many times for same object. (12 times for same ref)
My fix added caching object name to avoid needless
Object.prototype.toString.apply(value) === '[object ' + typeName + ']'
.We just can get object name only once.
Test plan