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

Excessive compile time for HTML entity resolution #763

Closed
hanna-kruppe opened this issue Jun 20, 2024 · 3 comments · Fixed by #764
Closed

Excessive compile time for HTML entity resolution #763

hanna-kruppe opened this issue Jun 20, 2024 · 3 comments · Fixed by #764

Comments

@hanna-kruppe
Copy link

hanna-kruppe commented Jun 20, 2024

Updating from quick-xml v0.31.0 to v0.32.0, I was surprised to see my project's build suddenly take longer. While quick-xml v0.31.0 takes less than three seconds to compile with optimizations, v0.32.0 takes between 10 and 30 seconds depending on how the stars align. This turns it from "beneath notice" into "the one thing that holds up everything" -- especially because I use Cargo profile overrides to enabled optimizations for quick-xml even for debug builds, so tests that have to deal with a lot of XML don't take forever. I did some digging and concluded that this is due to a single function (resolve_html5_entity) whose enormous control flow graph causes several LLVM passes to take several seconds each.

This is not a new problem: the function has existed in the same form for a long time. I've only noticed it now because it used to be feature-gated until 10d1ff8 and I had never enabled the escape-html feature. But v0.31.0 with that feature enabled compiles just as slowly as v0.32.0 does by default. So the easy workaround (modulo semver concerns) would make that function feature-gated again. Another workaround would be slapping #[inline] on it so that it's not lowered to LLVM IR if downstream crates never call it, but that's silly for other reasons. I assume y'all had reasons to make this function available unconditionally, but as long as it has such a hefty build time impact, I'd appreciate a way to side-step it.

Of course, it would be best to address the root of the problem (the huge match statement), though that may be more involved. The standard solution in such cases is to convert it into a data structure of some sort, so the lookup doesn't require O(n) code for n key-value pairs. If the function is hot, a well-tuned perfect hash table will probably also improve performance. From skimming the code that gets generated right now, it seems to do a jump table on entity.len() followed by a lot of byte-by-byte comparisons/jump tables. While that probably works pretty well if you see the same couple of entities repeatedly, I wouldn't expect it to be competitive in more complex workloads.

@Mingun
Copy link
Collaborator

Mingun commented Jun 20, 2024

Hm. This is interesting consequence of making long function public, I've never think about that. It was made public just for convenience of possible users, we can hide it under feature again.

@Mingun
Copy link
Collaborator

Mingun commented Jun 21, 2024

I just released 0.33.0 with this fix so you can save some time for your life 😃

@hanna-kruppe
Copy link
Author

Thanks!

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 a pull request may close this issue.

2 participants