Hooks: Micro-optimize runHooks

[aduth]

Foreseeing runHooks becoming a hot code path, I took to making a few small micro-optimizations, improving the handled and unhandled hook performance by approximately 42% and 172% respectively on my machine (handled meaning at least one hook handler exists). Standard benchmarking caveats apply (Node V8 engine, machine-specific, benchmarks themselves limited in real-world applicability).

Before:

handled x 5,839,917 ops/sec ±0.32% (91 runs sampled)
unhandled x 7,967,106 ops/sec ±0.63% (89 runs sampled)

After:

handled x 8,306,775 ops/sec ±0.46% (92 runs sampled)
unhandled x 22,606,859 ops/sec ±0.65% (90 runs sampled)

Optimizations include:

• Removing unnecessary code (a0e6eb6)
  • Technically, this removal would allow for doAction( '__current' ) to attempt to find handlers for the internal property, though would fail after the second condition, since array would not have .handlers property. To me, it's more concerning that we're allowing for this special key to exist with special treatment in the same scope as other hook handlers.
• Avoid assignment when either unnecessary or redundant (71bd540, 9177da1)
• Performant alternatives (truthy property access on null-prototype object vs. hasOwnProperty, faa0ed3, jsperf)
  • The reason for Object.create( null ) is to avoid issues with prototype property access:
    • !! ( {} ).valueOf // true
    • !! Object.create( null ).valueOf // false

Try yourself:

npm install
npm run build
node packages/hooks/benchmark

[codecov]

Codecov Report

Merging #85 into master will decrease coverage by 1.77%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   75.23%   73.45%   -1.78%     
==========================================
  Files          34       35       +1     
  Lines         432      437       +5     
  Branches       84       82       -2     
==========================================
- Hits          325      321       -4     
- Misses         89       98       +9     
  Partials       18       18

Impacted Files	Coverage Δ
packages/hooks/benchmark/index.js	0% <0%> (ø)
packages/hooks/src/createRemoveHook.js	95.45% <100%> (ø)	⬆️
packages/hooks/src/createHooks.js	100% <100%> (ø)	⬆️
packages/hooks/src/createHasHook.js	100% <100%> (ø)	⬆️
packages/hooks/src/createAddHook.js	100% <100%> (ø)	⬆️
packages/hooks/src/createDidHook.js	75% <100%> (ø)	⬆️
packages/hooks/src/createRunHook.js	94.11% <87.5%> (-5.89%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e116f2a...8d5854b. Read the comment docs.

[adamsilverstein]

This removes all validation from hooks names when applying them.

[aduth]

This removes all validation from hooks names when applying them.

What's the worst case scenario here?

The assumption I'd made, noted in extended commit description of a0e6eb6, is that validation of the hooks takes place when they're added, and if no hooks exist which match the hook name (a given if it's an invalid hook name), nothing would occur.

[adamsilverstein]

Seems reasonable 👍

[adamsilverstein]

Not clear why this is changing this here, is this performance related or required for other changes here?

[aduth]

Yes, from original comment:

• Performant alternatives (truthy property access on null-prototype object vs. hasOwnProperty, faa0ed3, jsperf)
  • The reason for Object.create( null ) is to avoid issues with prototype property access:
    • !! ( {} ).valueOf // true
    • !! Object.create( null ).valueOf // false

[adamsilverstein]

Ok, got it - I didn't see that this was required for the other changes to work. Nice!

[adamsilverstein]

Looks good to me!