Skip to content
/ lxml Public

expose collect_ids for HTMLParser as well#216

Merged
scoder merged 1 commit intolxml:masterfrom
plq:arskom
Dec 10, 2016
Merged

expose collect_ids for HTMLParser as well#216
scoder merged 1 commit intolxml:masterfrom
plq:arskom

Conversation

@plq
Copy link
Contributor

@plq plq commented Dec 4, 2016

It's said to lower the memory footprint of the parser at the expense of increased CPU load. Some use cases might care more about the amount memory consumed than CPU time, so it might make sense to expose this parameter.

- compact - save memory for short text content (default: True)
- default_doctype - add a default doctype even if it is not found in the HTML (default: True)
- collect_ids - Set this to False to avoid ever-growing id cache in
long-running processes (at a bit of a performance cost)
Copy link
Member

@scoder scoder Dec 6, 2016

Choose a reason for hiding this comment

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

That reads a bit biased. The downside is that you'd loose fast ID lookups, that should be mentioned instead.

@plq
Copy link
Contributor Author

plq commented Dec 8, 2016

did some rewording. you can chage it to your heart's content after your merge it as well, this is a fairly simple patch.

@scoder scoder merged commit 7b175fc into lxml:master Dec 10, 2016
@scoder
Copy link
Member

scoder commented Dec 10, 2016

Hmm, did you actually test if this has an impact? The code says that it shouldn't.

@plq
Copy link
Contributor Author

plq commented Dec 15, 2016

Well I'm trying to troubleshoot a memory leak myself and having close_ids=false in my daemon with this patch doesn't seem to have an impact.

@plq
Copy link
Contributor Author

plq commented Dec 15, 2016

More specifically, the leak is still there and having this flag doesn't seem to have slowed down the rate of the leak

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.

2 participants