-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
assert: optimize string length calculation in getSimpleDiff #60331
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
base: main
Are you sure you want to change the base?
assert: optimize string length calculation in getSimpleDiff #60331
Conversation
Reduced redundant typeof operations by caching type checks and simplified the string length calculation using ternary operators for improved readability.
BridgeAR
left a comment
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 calls will likely be inlined during optimization passes in the jit. I am fine with it either way.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60331 +/- ##
==========================================
- Coverage 88.59% 88.56% -0.03%
==========================================
Files 704 704
Lines 208474 207800 -674
Branches 40067 40035 -32
==========================================
- Hits 184696 184045 -651
- Misses 15805 15806 +1
+ Partials 7973 7949 -24
🚀 New features to boost your workflow:
|
ovflowd
left a comment
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.
I don't think this PR improves on anything. As @BridgeAR mentioned, these checks get optimized anyays.
IMO, these changes make it harder to read instead of actually easier. I'm -1 with these changes.
Nevertheless, appreciate the contribution 🙇
I agree that V8 will optimize repeated typeof checks and that this isn’t a performance-critical hot path, so I’m not claiming measurable speedups here. I mainly extracted the typeof results into named constants (isStrA, isStrE) to: Avoid repeating the same typeof expression in multiple places, Make the intent of the string-length adjustment a bit more explicit. That said, I completely agree that readability and maintainability should come first, especially in core code where future readers matter a lot more than speculative micro-optimizations. If folks feel the original version is clearer, I’m happy to drop or adjust this change—no strong preference here. 🙏 Also, when you have a moment, could you take a look at https://github.com/nodejs/node/pull/61223 |
Yeah, that's my main take, ternary operators can be helpful, but in this case, to my eyes, the older diff is clearer to read and to immediately see what's going on. You're spot on the DX over minimalism here.
Of couse :) |
Reduced redundant typeof operations by caching type checks
and simplified the string length calculation using ternary
operators for improved readability.