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

replace util.isType() with typeof #607

Closed
micnic opened this issue Jan 26, 2015 · 17 comments
Closed

replace util.isType() with typeof #607

micnic opened this issue Jan 26, 2015 · 17 comments
Labels
util Issues and PRs related to the built-in util module.

Comments

@micnic
Copy link
Contributor

micnic commented Jan 26, 2015

In the last PRs (#601 #605) I observed that the contributors replaced util.isString(data) with typeof data === 'string' or util.isFunction(callback) with typeof callback === 'function', the reason for these changes are some performance optimizations, I guess these optimizations are quite insignificant, but they are still improving the performance of the code. From my point of view it's also a code style change, so my question is: Should we allow such changes in the code style? if it's "yes" then we should replace them everywhere where util.isType() is used.

cc TC members @chrisdickinson @bnoordhuis

@rvagg
Copy link
Member

rvagg commented Jan 26, 2015

I'm interested in this question too, this is rolling back the introduction of these helpers that happened some time during 0.10 or 0.11 iirc and lead to core-util-is. The inconsistency, or at least the lack of a guiding principle bothers me a bit.

@bnoordhuis
Copy link
Member

I personally never liked the util.is*() functions, they're inefficient and unidiomatic. I won't be sad to see them go.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2015

I don't care for the helpers that compare to a constant/literal (null) or directly against the result of typeof. I would be less opposed to something like isUnsigned() that isn't as immediately obvious.

@mscdex
Copy link
Contributor

mscdex commented Jan 26, 2015

I did a test a few days ago where I replaced all instances of the util.is* helpers with the inline checks and performance-wise the gain was only a few percent difference here and there in the benchmarks. I would've guessed that v8 would normally inline them, but I suppose there could be cases where functions that use the helpers get deoptimized, so inlining can't occur then?

I'm not sure about an isUnsigned() though, what kind of logic would be in that function? Does that mean checking if v8 is storing the number as an unsigned integer internally or just that value > 0?

@vkurchatkin
Copy link
Contributor

Doesn't inlining requires a check? Like, check that util.isSomething is still the same object

@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2015

@mscdex I was thinking n >>> 0 === n for identifying UInt32 types, which are somewhat common.

@bnoordhuis
Copy link
Member

@vkurchatkin That's right. It looks something like util && typeof(util) === 'object' && hiddenMap(util) === utilMap && typeof(util.isSomething) === 'function' && hiddenMap(util.isSomething) === isSomethingMap in pseudo-code. The individual checks aren't that expensive but in aggregate, they add up.

@vkurchatkin
Copy link
Contributor

@bnoordhuis I wonder if const isSomething = util.isSomething would help

@bnoordhuis
Copy link
Member

@vkurchatkin It could in theory but I don't think V8 is currently smart enough to eliminate the checks completely. Don't quote me on that though, I may be operating on outdated data. :-)

@piscisaureus
Copy link
Contributor

I personally never liked the util.is*() functions, they're inefficient and unidiomatic.

Neither did I, so I won't be sad to see them go.
However why were they introduced? @bnoordhuis I always thought they were your doing.

@Fishrock123
Copy link
Contributor

Looks like they replaced at-build-macros for the same checks: 22c68fd

@mscdex
Copy link
Contributor

mscdex commented Jan 26, 2015

Initially the inline checks were replaced with macros in 0330bdf and the macros were converted to the util.is* functions a couple of days later in 22c68fd.

EDIT: @Fishrock123 beat me to it ;-)

@feross
Copy link
Contributor

feross commented Jan 26, 2015

I assume we're only discussing whether core should continue to use util.isString, etc. These methods ought to remain in util forever or we'll break hundreds or thousands of modules.

@mscdex
Copy link
Contributor

mscdex commented Jan 26, 2015

Yes, I don't think there is any harm keeping the convenience methods in util itself, but core should benefit (at least somewhat speed-wise) from not using them itself.

@brendanashworth
Copy link
Contributor

I think it should also be mentioned in the docs that they aren't in use in core and just delimit their use to instanceof and typeof.

@brendanashworth brendanashworth added the util Issues and PRs related to the built-in util module. label Jan 27, 2015
@seishun
Copy link
Contributor

seishun commented Jan 28, 2015

I assume we're only discussing whether core should continue to use util.isString, etc. These methods ought to remain in util forever or we'll break hundreds or thousands of modules.

These methods weren't even documented until 95955a1. If anyone used them before that they were asking for trouble.

cjihrig added a commit that referenced this issue Feb 1, 2015
Many of the util.is*() methods used to check data types
simply compare against a single value or the result of
typeof. This commit replaces calls to these methods with
equivalent checks. This commit does not touch calls to the
more complex methods (isRegExp(), isDate(), etc.).

Fixes: #607
PR-URL: #647
Reviewed-By: Ben Noordhuis <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2015

Closed in 6ac8bdc

@cjihrig cjihrig closed this as completed Feb 1, 2015
@rvagg rvagg removed the tc-agenda label Feb 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests