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

Fix wildcard hx-on search's root node #2060

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

Telroshan
Copy link
Collaborator

@Telroshan Telroshan commented Nov 30, 2023

Description

As @svenberkvens pointed out in #2019, the XPATH query run to find hx-on wildcard elements to process, as well as its IE11 compatible code counterpart, didn't take the root node parameter into account, resulting in scanning the whole DOM to find such elements.
This was causing a huge performance impact for each individual node processed later on by htmx on a initially large DOM.

This PR fixes this issue by looking for hx-on wildcard elements from the root element parameter, instead of from the document itself.

Corresponding issues: #2019 and #2051

Testing

Ran a performance test using htmx version before the introduction of that hx-on wildcard attribute (i.e. 1.9.2), one with the current version, and another one with this PR's changes
The test creates 100 000 links in the DOM, then adds a dummy element and calls htmx.process on it

Took 7.3 ms to process node

Took 591 ms to process node

Took 7.9 ms to process node

So it looks like we're back to the initial performance of processing an individual node with a very large DOM around.

Also ran the test suite locally which works, no no regression seems to be introduced by this change

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

Comment on lines +1916 to +1919
if (shouldProcessHxOn(elt)) {
elements.push(elt)
}

Copy link
Collaborator

@alexpetros alexpetros Nov 30, 2023

Choose a reason for hiding this comment

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

I'm not convinced that that any of the shouldProcessHxOn part is necessary. Doesn't the performance impact primarily come from the lines 1921 and 1924?

Copy link
Collaborator

@alexpetros alexpetros Nov 30, 2023

Choose a reason for hiding this comment

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

Could you run your test suite against the two-line version of this PR, that only makes the changes on lines 1921 and 1924?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexpetros Running the xpath query from the element itself with that leading . syntax unfortunately doesn't process the root node itself. Same goes for getElementsByTagName in the IE fallback code that will get all children, but not the root node itself, thus this first check to add the root itself to the array if it should be processed for hx-on attributes.

Removing this if breaks a lot of hx-on tests btw

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then the function is just there to avoid duplicating the logic from the IE fallback code + the root node check

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I understand you moved it—I didn't see the delete bit below.

@alexpetros alexpetros added the ready for review Issues that are ready to be considered for merging label Nov 30, 2023
@1cg 1cg merged commit 98997bd into bigskysoftware:dev Nov 30, 2023
1 check passed
@Telroshan Telroshan deleted the fix-hx-on-context branch December 1, 2023 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ready for review Issues that are ready to be considered for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants