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

Added feature flag atomic to make use of atomic StrTendril type. #102

Merged
merged 3 commits into from Feb 28, 2023

Conversation

jaboatman
Copy link
Contributor

Enabling this feature allows scraper::Html and associated types to implement Send.

@cfvescovo
Copy link
Collaborator

Considering this for the next minor release

@cfvescovo
Copy link
Collaborator

cfvescovo commented Feb 25, 2023

cc @teymour-aldridge

@adamreichold
Copy link
Contributor

adamreichold commented Feb 26, 2023

@jaboatman Do you have any experience on the resulting performance impact?

(We currently live with the !Send bound in a multi-threaded async code base, but this repeatedly trips up my co-workers when they add a yield point while a Html is alive. Hence, I am wondering whether this would be worth it just for the ergonomics.)

@j-mendez
Copy link
Contributor

@jaboatman changes look good. I was actually about to start adding these changes. Minor - instead of a util function an interface using “.into” or “from”.

src/lib.rs Show resolved Hide resolved
@cfvescovo cfvescovo removed the request for review from teymour-aldridge February 26, 2023 19:27
@jaboatman
Copy link
Contributor Author

@jaboatman Do you have any experience on the resulting performance impact?

(We currently live with the !Send bound in a multi-threaded async code base, but this repeatedly trips up my co-workers when they add a yield point while a Html is alive. Hence, I am wondering whether this would be worth it just for the ergonomics.)

I don't, I would imagine you'd need to benchmark it for your specific use case. We needed to store Html across await points which is why I implemented it. In our setting high performance isn't critical and as such I haven't noticed any problems with using the atomic flag.

@j-mendez
Copy link
Contributor

@jaboatman changes look good. I was actually about to start adding these changes. Minor - instead of a util function an interface using “.into” or “from”.

Can actually just use SendTendril instead. https://github.com/causal-agent/scraper/pull/102/files#diff-af08c3181737aa5783b96dfd920cd5ef70829f46cd1b697bdb42414c97310e13R259 needs an update after the last performance commits if possible.

@adamreichold
Copy link
Contributor

Can actually just use SendTendril instead.

I don't think this will work as SendTendril has a significantly reduced API surface which is the price it pays for being Send without changing the Atomicity type parameter.

@jaboatman
Copy link
Contributor Author

https://github.com/causal-agent/scraper/pull/102/files#diff-af08c3181737aa5783b96dfd920cd5ef70829f46cd1b697bdb42414c97310e13R259 needs an update after the last performance commits if possible.

Done.

@cfvescovo
Copy link
Collaborator

LGTM
@adamreichold @j-mendez are there any changes you would like to see implemented?

@j-mendez
Copy link
Contributor

LGTM @adamreichold @j-mendez are there any changes you would like to see implemented?

LG2M thanks for asking.

@adamreichold
Copy link
Contributor

LGTM as well. I am looking forward to the new release! Thank your work maintaining this crate.

@cfvescovo cfvescovo merged commit ec91bf1 into causal-agent:master Feb 28, 2023
@j-mendez
Copy link
Contributor

Thank you for maintaining this crate as well @cfvescovo !

@cfvescovo
Copy link
Collaborator

Thank you for your contributions!

@OlivierH
Copy link

Thanks for this!
Note that this is not currently showing in the docs.

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.

None yet

5 participants