Deprecated: Improve $.trim performance for strings with lots of whitespace #461
Conversation
|
|
vlsi
force-pushed
the
faster_trim
branch
2 times, most recently
from
001ac34 to
185d4b5
Compare
|
Thanks for the PR. With Migrate, we're more concerned about backwards compatibility than performance. This is why implementation of various methods is based on how they were implemented in jQuery before deprecation/removal. The implementation here shouldn't be changed before the upstream version in jQuery is updated; you can find it at https://github.com/jquery/jquery/blob/3.x-stable/src/deprecated.js. One advantage of making a change in jQuery first is that it has a more comprehensive test suite so any mistakes in the implementation would be detected better. That said, since the util is deprecated, we'd rather not increase the library size to address an edge case that people shouldn't rely on anyway as we recommend migrating to native Since any action should first be taken in jQuery upstream, I'm going to close the PR. If a change to jQuery gets accepted, we can then backport the change here. (note: when submitting a PR, please address any checks errors GitHub throws at you; for example, this PR had a lot of code style errors) |
|
@mgol , the exact
My edge case is that my JIRA instance takes 20 seconds to display an issue (it spends most of that time within The implementation is really straightforward, and it doesn't make jquery-migrate significantly bigger.
I am OK to fix the issues if you merge the PR, however,
|
|
It was removed from the
This project is volunteer-driven, I don't have active support contract with anyone. And it so happened that I was on vacation recently and before the vacation I was busy at work with hardly any time for open source work. I'm reviewing PRs when I have time. Usually it doesn't take two months but from time to time I have less time for OSS; you can't expect me to always reply within a given time frame.
I explained why changes need to land in jQuery upstream first, at least as long as the code is still maintained there which is the case here. My time is limited. I'd be happy to review your PR to jQuery. When you submit a PR to an OSS project, you can't always expect it will get merged; sometimes it doesn't align with the project priorities. In case of jQuery, if you want to avoid spending time on CI issues only to see your PR rejected, it's a good idea to open an issue first, present a concrete use case (like the JIRA example you mentioned here - without a concrete case like that, we can't know if the issue is worth fixing) and wait for the team to approve the approach; then your PR may still get requests for changes but will generally be merged when those are addressed. For the record, I think this would be a good change to land, although we'd like to compress the code a bit. It's also likely that we'd prefer to use the regex approach in all browsers to make the code smaller; that should still resolve your performance concerns as much as I understand. |
|
@vlsi Since jquery/jquery#5068 landed, I'm going to reopen this PR. We can merge it if you update the implementation here to match the one that landed in the jQuery PR. |
There was a problem hiding this comment.
Marking as "Request changes" until the implementation is updated to match the jQuery one from jquery/jquery#5068.
…s of whitespaces The old implementation took O(N^2) time to trim the string when multiple adjacent spaces were present. For instance, consider the string "A B" (10 spaces between A and B). Then old regexp /[\s]+$/ would take 10 steps until it realizes the regexp does not match at position 1. Then it would try to match the regexp at position 2, and it would take 9 steps. Then 8 steps for position 3, ... so it would take 10*10/2 steps until it figures out the regexp does not match. See jquery/jquery#5068 The new approach is to require "non-whitespace" char before the whitespace run, so it spends just one step for each space in the string.
mgol
changed the title
The old implementation took O(N^2) time to trim the string when multiple adjacent spaces
were present.
For instance, consider the string "A B" (10 spaces between A and B).
Then old regexp /[\s]+$/ would take 10 steps until it realizes the regexp does not match at position 1.
Then it would try to match the regexp at position 2, and it would take 9 steps.
Then 8 steps for position 3, ... so it would take 10*10/2 steps until it figures out the regexp does not match.
The new approach is to require "non-whitespace" char before the whitespace run, so it spends just one step for each space in the string.
Note: if
String.prototype.trim()is good enough, then the implementation would use it.