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

Deprecated: Improve $.trim performance for strings with lots of whitespace #5068

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

vlsi
Copy link

@vlsi vlsi commented Jun 27, 2022

The old implementation took O(N^2) time to trim the string when multiple adjacent spaces
were present.

In my case, the issue impacts JIRA which takes ~20 seconds to display an issue where most of the time is spent in jQuery.trim.

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.

@vlsi vlsi force-pushed the faster_trim branch 2 times, most recently from 61fb761 to 66018e1 Compare June 27, 2022 18:01
@vlsi vlsi changed the title Performance: improve .trim performance for large strings contains lots of whitespaces Performance: improve .trim performance for large strings containing lots of whitespaces Jun 28, 2022
@vlsi vlsi changed the title Performance: improve .trim performance for large strings containing lots of whitespaces Performance: improve .trim performance for large strings containing lots of whitespace Jun 28, 2022
@mgol
Copy link
Member

mgol commented Jul 12, 2022

Thanks for the PR. This is adding 63 bytes to the gzipped minified build, we consider that a significant number. As I suggested in the other thread, can you try removing the branch that leverages native trim when available to decrease the size impact? Are there any issues with this strategy?

To verify the size increase, please run npm run build locally on the 3.x-stable branch and then on your PR rebased over that branch; you should see at the end what's the size of your current build compared to 3.x-stable - the number 68 I posted above.

@vlsi
Copy link
Author

vlsi commented Jul 12, 2022

can you try removing the branch that leverages native trim when available to decrease the size impact?

Do you mean I should completely remove native trim approach?

@mgol
Copy link
Member

mgol commented Jul 12, 2022

Do you mean I should completely remove native trim approach?

Yes, that was the idea. Are there any issues with this approach? As I understand, your regex changes should resolve the performance issues that you reported.

You can also try the version without a native trim on a separate branch, just to measure what size impact would this version have while still being able to go back to the version you currently proposed if needed.

@vlsi
Copy link
Author

vlsi commented Jul 13, 2022

Native trim should be faster than any regexp, and I think the recent browsers should not require the regexp.
Frankly speaking, I think going for native trim is worth doing as it would make the webpages faster for the end-user.

Just in case:

#baseline
   raw     gz Sizes
289641  85451 dist/jquery.js
 89653  30945 dist/jquery.min.js

# with regexp only (without native trim)
   raw     gz Sizes
290048  85627 dist/jquery.js
 89691  30959 dist/jquery.min.js
  +407   +176 dist/jquery.js
   +38    +14 dist/jquery.min.js

# regexp and native trim
   raw     gz Sizes
290297  85690 dist/jquery.js
 89812  31008 dist/jquery.min.js
  +656   +239 dist/jquery.js
  +159    +63 dist/jquery.min.js

# regexp, single trim function that checks useDefaultTrim always
   raw     gz Sizes
290226  85687 dist/jquery.js
 89781  31006 dist/jquery.min.js
  +585   +236 dist/jquery.js
  +128    +61 dist/jquery.min.js

@vlsi
Copy link
Author

vlsi commented Jul 13, 2022

Here's a sample benchmark: https://jsbench.me/oml5j9kjfu/1

Note that even the optimized regex still needs to walk over the entire string while the native implementation can walk the string backwards.

Results on my machine are:

trim 1000 char text like __________ __________ __________ __________ __________ __________ __________ __________ __________ __________ __________ __________ ...

let n = 1;
let wordLen = 10;
let numWords = 100;
let spaceAround = 1;
let mid = ('_'.repeat(wordLen) + ' ').repeat(numWords) + '_';
let src = ' '.repeat(spaceAround) + mid + ' '.repeat(spaceAround);
let ltrim = /^[\s\uFEFF\xA0]+/;
let rtrim = /([^\s\uFEFF\xA0])[\s\uFEFF\xA0]+$/;
native: 16M ops/sec ~ 0.06 ms/trim
regex: 700K ops/sec ~ 1.4 ms/trim (~23 times slower)

trim ' '.repeat(1000) + '_' + ' '.repeat(1000)

native: 750K ops/sec ~ 1.3 ms/trim
regex: 680K ops/sec ~ 1.47 ms/trim (13% slower)

trim ' _ '

native: 85M ops/sec ~ 13 us/trim
regex: 5.5M ops/sec ~ 180 us/trim (~13 times slower)

Frankly speaking, I think the results show that performance improvement is worth the added 63 bytes.
So I believe regex-based implementation could be a not-so-bad workaround for stale browsers, while native trim would save users's CPU and battery (which is more important than 63-byte transfer).

WDYT?

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test cases.

I added some comments that should contribute to making this patch much smaller.

It's a bit tricky as this is a deprecated util that should generally not be used at all... The use case presented at jquery/jquery-migrate#461 (comment) says something about a slow JIRA instance. I wonder: would updating jQuery with this patch help here, considering that JIRA would have to update the included jQuery?

That said, I'd be leaning towards accepting this change with my suggestions as trimming is a pretty common operation and I imagine $.trim must be used a lot, especially in older code. My suggestions should make the size increase significantly down.

Thoughts, @timmywil @dmethvin @gibson042?

src/deprecated.js Outdated Show resolved Hide resolved
src/deprecated.js Outdated Show resolved Hide resolved
src/deprecated.js Outdated Show resolved Hide resolved
@mgol mgol added Deprecated 3.x-only Needs info Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. and removed Needs info labels Jul 13, 2022
@vlsi
Copy link
Author

vlsi commented Jul 13, 2022

Here's profiling of JIRA issue I run into. I'm running Chrome 103.0.5060.114 on Apple M1 Max. There are two jquery.trim executions, and each of them takes 5 seconds.

JIRA freeze

@vlsi
Copy link
Author

vlsi commented Jul 13, 2022

I've merged functions into one:

   raw     gz Sizes
290221  85685 dist/jquery.js
 89781  31006 dist/jquery.min.js
  +580   +234 dist/jquery.js
  +128    +61 dist/jquery.min.js

@vlsi
Copy link
Author

vlsi commented Jul 13, 2022

I've created deprecated/support, and the size is

   raw     gz Sizes
290171  85668 dist/jquery.js
 89767  30997 dist/jquery.min.js
  +530   +217 dist/jquery.js
  +114    +52 dist/jquery.min.js

@vlsi
Copy link
Author

vlsi commented Jul 13, 2022

I wonder: would updating jQuery with this patch help here, considering that JIRA would have to update the included jQuery?

Of course, it would definitely help.
If I remember well, then the optimized regex improves the performance from 5 seconds to 5 milliseconds. Unfortunately, the input data has sensitive information, and I would like to avoid spending time distilling the test case since the optimization is kind of obvious.

@vlsi vlsi force-pushed the faster_trim branch 5 times, most recently from 81d37f7 to 0621c3e Compare July 13, 2022 11:27
@mgol mgol requested review from dmethvin and gibson042 July 13, 2022 11:33
@vlsi
Copy link
Author

vlsi commented Jul 14, 2022

   raw     gz Compared to 3.x-stable @ 0f6c3d9efc5f7e844bdcf8ef44f9327f414bea77
  +102    +28 dist/jquery.js
   +53    +29 dist/jquery.min.js

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments, otherwise looks good! I'm also happy where this landed size-wise.

test/unit/support.js Outdated Show resolved Hide resolved
src/deprecated/support.js Outdated Show resolved Hide resolved
test/unit/support.js Outdated Show resolved Hide resolved
test/unit/support.js Outdated Show resolved Hide resolved
@gibson042
Copy link
Member

gibson042 commented Jul 14, 2022

Adding 29 bytes for performance that only matters with pathological input seems like too much to me, but I would be comfortable with still improving performance by orders of magnitude for the earlier 6-byte increase:

@@ -15,7 +15,7 @@ define( [
 
 // Support: Android <=4.0 only
 // Make sure we trim BOM and NBSP
-var rtrim = /^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/g;
+var rtrim = /^[\s\uFEFF\xA0]+|([^\s\uFEFF\xA0])[\s\uFEFF\xA0]+$/g;
 
 // Bind a function to a context, optionally partially applying any
 // arguments.
@@ -82,6 +82,6 @@ jQuery.isNumeric = function( obj ) {
 jQuery.trim = function( text ) {
        return text == null ?
                "" :
-               ( text + "" ).replace( rtrim, "" );
+               ( text + "" ).replace( rtrim, "$1" );
 };
 } );

@vlsi vlsi force-pushed the faster_trim branch 3 times, most recently from 6051fc7 to 159f9c1 Compare July 14, 2022 16:46

"use strict";

support.trim = "" === "\uFEFF\xA0".trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I am arguing against the introduction of support.trim at +29 bytes (and really, even at +10).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me personally the biggest issue with size increases is they creep in, adding up to a meaningful increase over time. In this case, this danger does not exist as this is deprecated code that's already removed in 4.0.0-pre. That makes +29 bytes more acceptable to me.

BTW, we could still make it smaller by dropping \xA0 - this is actually trimmed properly by Android 4.0, \uFEFF is not, I just checked. That lowers the size increase to +24 bytes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native implementation is still can be an order of magnitude faster, and trim is used a lot in the wild.
So it looks wrong to default to regexp implementation which is needed for a single stale browser only.

I agree that keeping the dependencies small is important, however, I would say that battery life might be even more important than 29 bytes of extra transfer.

Code size is easy to compare as long as npm prints it. However, it would be nice to have benchmarks that would capture energy consumed by "real-life websites".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you claiming that a real-life website, let alone a significant number of them, are spending so much time in jQuery.trim that an improvement from O(n²) to O(n) (with real effect of 240x for input with hundreds of medial whitespace characters and 23000x for input with tens of thousands, taking the OP 20 seconds down to about 1 ms) is not sufficient to address the overall experience? I'm not denying that native String.prototype.trim is astronomically faster still, but how would such sites even get the input data fast enough for that further improvement to make a material difference?

That said, I'm not going to panic over 18 bytes in a legacy branch. If @mgol is in favor, that's fine with me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A real site, can and should, of course, call the native trim() method directly if its product requirements allows that, and could do so, at least for the handful of rare cases where this is known to make a significant difference.

While sites may be embedding a particular jQuery version of a variety of different reasons (e.g. blocked on migration of code, or offer jQuery API to other plugins, or browser support for one of the legacy browsers, such as the case for Wikipedia where the JS payload supports IE11 and Android 5 but not IE9 or Android 4). It's quite likely a consumer could even adopt String-trim unconditionally in its code, if all its clients support ES5, even if jQuery itself maintains wider support.

@timmywil timmywil added this to the 3.6.1 milestone Jul 18, 2022
@timmywil
Copy link
Member

Thanks for your contribution @vlsi! We discussed this in the meeting and we'd prefer to go with @gibson042's shorter solution and not add support.trim to 3.x this late in the game. If you can make that change, we'd be happy to get this into 3.6.1.

@vlsi
Copy link
Author

vlsi commented Jul 19, 2022

For reference, I've captured the problematic text (~900KiB).

Old regexp takes ~5 seconds.
The updated one takes 3ms.

For trivial cases (e.g. trim('<div>')), the updated regexp executes with a rate of ~11M/sec (the native executes at a rate of 130M/sec).
In other words, 10K trims would consume 1ms of CPU time.

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good to me now.

@mgol
Copy link
Member

mgol commented Jul 19, 2022

@gibson042 since your "Request changes" review is pending - can you have a look and approve if you have no remarks? This now implements your +6 bytes proposal, as I understand.

@mgol mgol requested a review from gibson042 July 19, 2022 17:38
@mgol mgol removed the Needs info label Jul 19, 2022
… of whitespace runs

Regex imp implementation takes O(N^2) time to trim the string when multiple
adjacent spaces were present.

The new expression require that the "whitespace run" starts from a non-whitespace
to avoid O(N^2) behavior when the engine would try matching "\s+$" at each space position.
@mgol mgol removed Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Jul 20, 2022
@mgol mgol changed the title Performance: improve .trim performance for large strings containing lots of whitespace Deprecated: Improve $.trim performance for strings with lots of whitespace Jul 20, 2022
@mgol mgol merged commit 6994010 into jquery:3.x-stable Jul 20, 2022
@mgol
Copy link
Member

mgol commented Jul 20, 2022

Landed, thanks! This change will be included in the upcoming 3.6.1 release.

vlsi added a commit to vlsi/jquery-migrate that referenced this pull request Jul 20, 2022
…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 pushed a commit to jquery/jquery-migrate that referenced this pull request Jul 20, 2022
…space

Regex imp implementation takes `O(N^2)` time to trim the string when
multiple adjacent spaces were present.

The new expression require that the "whitespace run" starts from
a non-whitespace to avoid `O(N^2)` behavior when the engine would
try matching `\s+$` at each space position.

Closes gh-461
Ref jquery/jquery#5068
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants