inspect-js / is-string

Is this value a JS String object or primitive? This module works cross-realm/iframe, and despite ES6 @@toStringTag.
MIT License
20 stars 3 forks source link

Slow on the try/catch case #24

Open joepie91 opened 3 months ago

joepie91 commented 3 months ago

While profiling an application, I found is-string (and a few related libraries) pretty much topping the charts across the entire codebase:

image

It appears that the try/catch check in tryStringObject does not get optimized (due to being a try/catch, I assume) and this makes the type checks very slow, comparatively speaking. It's on a hot path for me due to being used in argument validation code.

The same problem manifests in is-number-object and is-date-object (and presumably others that happen to not be used in this particular project).

Perhaps this check could be replaced with a faster one in some way, or if not, only applied if the value hits a (faster) test that suggests this check should be applied?

ljharb commented 3 months ago

hmm, the intention behind putting the try/catch inside a separate function wad that it allows v8 to optimize the main function. What browsers is this profiled in?

The current implementation already does all the fast checks it can - typeof, mainly - before hitting the try/catch (i could add an additional truthiness check to account for null, but i doubt that’ll help much)

Do you know what parts of your application are calling the predicates?

joepie91 commented 3 months ago

hmm, the intention behind putting the try/catch inside a separate function wad that it allows v8 to optimize the main function.

That part seems to be working; the main functions do not meaningfully show up on the profiler. But the separate try/catch functions are themselves still slow/unoptimized enough to take a comparatively large amount of time :)

What browsers is this profiled in?

This was in Node.js 18.20.3, run with --inspect --inspect-brk and using Chromium developer tools to connect to the process (profiling with its Performance tab).

The current implementation already does all the fast checks it can - typeof, mainly - before hitting the try/catch (i could add an additional truthiness check to account for null, but i doubt that’ll help much)

I don't think that would help in my case - the case that I suspect it's triggering on a lot, is "argument that can be either a string or a number, and so one of the validators has to fail before it checks the next one", which means that it hits the no-match case quite often with non-null values.

Do you know what parts of your application are calling the predicates?

They're called from within the corresponding Validatem validators, which do the argument validation for my SPARQL query builder, so they get called quite a lot (one to multiple times for each query builder method). I control basically the entire stack right up to the inspect-js libraries, and there's nothing else slow inbetween.

For now I've vendored a modified version of these libraries that does an instanceof check instead (at the cost of no cross-realm support), and total runtime of the code as a whole (so not just the validation) was cut by about 30%.

ljharb commented 3 months ago

If you're going for a caveat-d approach, why not use typeof x === 'string' etc for the primitives?

The only reason to use the inspect-js predicate packages for primitives is to handle boxed primitives.

joepie91 commented 3 months ago

Well, the intention was to get as close as possible to a correct/comprehensive check :)

ljharb commented 3 months ago

instanceof isn't just about cross-realm, though - it can be faked with Symbol.hasInstance, and can be tricked by setting a .constructor property. Unfortunately the slow method here is the only way I know of to check the internal slot.