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

add clone to Xpath #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add clone to Xpath #50

wants to merge 1 commit into from

Conversation

claudenobs
Copy link

@claudenobs claudenobs commented Sep 1, 2024

This adds the clone Trait to Xpath so it can be used with the cached macro from the cached crate.

This PR is against 0.6.4 because the master branch currently has some serious performance regressions
(in my case html parsing takes ~2s instead of ~30ms)

@James-LG
Copy link
Owner

James-LG commented Sep 2, 2024

The html module has lots of problems since I hacked it together years ago. I've been working on rewriting it for the past few months which I published to #51 for visibility. It's a huge undertaking so it will take a while.

Your actual change looks perfectly reasonable, but I can't merge it with the out of date branch.

The performance regression comes from these chained replace() calls. Ideally it would do a single pass, but that turns out to be quite complicated, and it would feel like a waste of time given my html module rewrite would throw it all away.

So instead I have attempted to reduce how many times it is called in #52 as a temporary fix until the rewrite is complete. Let me know if the james/perf branch helps at all. If it does, I will merge that, you can update this branch, and then I will merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants